Reonboarding a term before calling cleanup breaks the offboard redemption pause mechanism

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L150-L199
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20Gauges.sol#L395-L42
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SimplePSM.sol#L146-L154

Vulnerability details

Summary

When a term is offboarded, redeeming CREDIT tokens is paused till all loans are closed and the cleanup function is called. If the term is re-onboarded before the cleanup function is called, redemption stays paused and can be manually unpaused by the governance, but the automatic pause / unpause mechanism of the offboarding process stays broken forever as the nOffboardingsInProgress variable will always be above 0.

Vulnerability Details

Offboarding a term goes through a voting process which updates the canOffboard mapping for the given term to true so that the offboard function can be called:

/// @notice Offboard a LendingTerm.
/// This will prevent new loans from being open, and will prevent GUILD holders to vote for the term.
/// @param term LendingTerm to offboard from the system.
function offboard(address term) external whenNotPaused {
  require(canOffboard[term], 'LendingTermOffboarding: quorum not met');

  // update protocol config
  // this will revert if the term has already been offboarded
  // through another mean.
  GuildToken(guildToken).removeGauge(term);

  // pause psm redemptions
  if (nOffboardingsInProgress++ == 0 && !SimplePSM(psm).redemptionsPaused()) {
    SimplePSM(psm).setRedemptionsPaused(true);
  }

  emit Offboard(block.timestamp, term);
}

As we can see if the nOffboardingsInProgress is currently 0 the setRedemptionsPaused function of the SimplePSM contract is called to pause the redemption of CREDIT tokens.

After that the cleanup function can be called (if there are no open loans) which will call setRedemptionsPaused again to unpause the redemption of CREDIT tokens if nOffboardingsInProgress - 1 equals 0:

/// @notice Cleanup roles of a LendingTerm.
/// This is only callable after a term has been offboarded and all its loans have been closed.
/// @param term LendingTerm to cleanup.
function cleanup(address term) external whenNotPaused {
  require(canOffboard[term], 'LendingTermOffboarding: quorum not met');
  require(LendingTerm(term).issuance() == 0, 'LendingTermOffboarding: not all loans closed');
  require(GuildToken(guildToken).isDeprecatedGauge(term), 'LendingTermOffboarding: re-onboarded');

  // update protocol config
  core().revokeRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, term);
  core().revokeRole(CoreRoles.GAUGE_PNL_NOTIFIER, term);

  // unpause psm redemptions
  if (--nOffboardingsInProgress == 0 && SimplePSM(psm).redemptionsPaused()) {
    SimplePSM(psm).setRedemptionsPaused(false);
  }

  canOffboard[term] = false;
  emit Cleanup(block.timestamp, term);
}

A term can also be re-onboarded if it was offboarded before, which is done by the addGauge function:

function _addGauge(uint256 _type, address gauge) internal returns (uint256 weight) {
  bool newAdd = _gauges.add(gauge);
  bool previouslyDeprecated = _deprecatedGauges.remove(gauge);
  // add and fail loud if zero address or already present and not deprecated
  require(gauge != address(0) && (newAdd || previouslyDeprecated), 'ERC20Gauges: invalid gauge');

  if (newAdd) {
    // save gauge type on first add
    gaugeType[gauge] = _type;
  } else {
    // cannot change gauge type on re-add of a previously deprecated gauge
    require(gaugeType[gauge] == _type, 'ERC20Gauges: invalid type');
  }

  // Check if some previous weight exists and re-add to total. Gauge and user weights are preserved.
  weight = getGaugeWeight[gauge];
  if (weight != 0) {
    totalTypeWeight[_type] += weight;
    totalWeight += weight;
  }

  emit AddGauge(gauge, _type);
}

If the cleanup function was not called before the term is re-onboarded, the nOffboardingsInProgress variable and the redemptionsPaused variable are not updated. Therefore, redemption stays paused and nOffboardingsInProgress at 1 or higher.

The redemptions can be manually unpaused by the governance:

/// @notice set `redemptionsPaused`
/// governor-only, to allow full governance to update the psm mechanisms,
/// or automated processes to pause redemptions under certain conditions.
function setRedemptionsPaused(bool paused) external onlyCoreRole(CoreRoles.GOVERNOR) {
  redemptionsPaused = paused;
  emit RedemptionsPaused(block.timestamp, paused);
}

But as the nOffboardingsInProgress variable is not updated, the automatic pause / unpause mechanism of the offboarding process stays broken forever as the nOffboardingsInProgress variable will always be above 0 and therefore if (nOffboardingsInProgress++ == 0 && !SimplePSM(psm).redemptionsPaused()) clause will never be reached to pause the redemption of CREDIT tokens.

The following POC can be implemented in the LendingTermOnboarding.t.sol test file (import {LendingTermOffboarding} from "@src/governance/LendingTermOffboarding.sol"; must be added to the imports first):

function testReOnboardFreeze() public {
  // add import {LendingTermOffboarding} from "@src/governance/LendingTermOffboarding.sol"; to the imports
  LendingTermOffboarding offboarder = new LendingTermOffboarding(address(core), address(guild), address(psm), _QUORUM);
  vm.startPrank(governor);
  core.grantRole(CoreRoles.GAUGE_REMOVE, address(offboarder));
  core.grantRole(CoreRoles.GOVERNOR, address(offboarder));
  vm.stopPrank();
  vm.label(address(offboarder), 'offboarder');

  LendingTerm term = LendingTerm(
    onboarder.createTerm(
      address(termImplementation),
      LendingTerm.LendingTermParams({
        collateralToken: address(collateral),
        maxDebtPerCollateralToken: _CREDIT_PER_COLLATERAL_TOKEN,
        interestRate: _INTEREST_RATE,
        maxDelayBetweenPartialRepay: 0,
        minPartialRepayPercent: 0,
        openingFee: 0,
        hardCap: _HARDCAP
      })
    )
  );
  vm.label(address(term), 'term');

  // mint GUILD & self delegate
  guild.mint(bob, _QUORUM);
  vm.prank(bob);
  guild.incrementDelegation(bob, _QUORUM);
  vm.roll(block.number + 1);
  vm.warp(block.timestamp + 13);

  // do onboard
  vm.startPrank(bob);
  uint256 proposalId = onboarder.proposeOnboard(address(term));
  (address[] memory targets, uint256[] memory values, bytes[] memory calldatas, string memory description) = onboarder
    .getOnboardProposeArgs(address(term));
  vm.roll(block.number + 1);
  vm.warp(block.timestamp + 13);
  onboarder.castVote(proposalId, uint8(GovernorCountingSimple.VoteType.For));
  vm.roll(block.number + _VOTING_PERIOD + 1);
  vm.warp(block.timestamp + 13);
  onboarder.queue(targets, values, calldatas, keccak256(bytes(description)));
  vm.roll(block.number + 1);
  vm.warp(block.timestamp + _TIMELOCK_MIN_DELAY + 13);
  onboarder.execute(targets, values, calldatas, keccak256(bytes(description)));
  vm.stopPrank();

  // check execution
  assertEq(guild.isGauge(address(term)), true);

  // borrow funds
  guild.mint(address(this), _HARDCAP * 2);
  guild.incrementGauge(address(term), _HARDCAP);

  uint256 aliceLoanSize = 500_000e18;
  collateral.mint(alice, aliceLoanSize);
  vm.startPrank(alice);
  collateral.approve(address(term), aliceLoanSize);
  bytes32 aliceLoanId = term.borrow(aliceLoanSize, aliceLoanSize);
  vm.stopPrank();

  // offboard
  vm.roll(block.number + 1);
  vm.warp(block.timestamp + 7 days + 13);

  // prepare (1)
  guild.mint(bob, _QUORUM);
  vm.startPrank(bob);
  guild.delegate(bob);
  uint256 snapshotBlock = block.number;
  offboarder.proposeOffboard(address(term));
  vm.roll(block.number + 1);
  vm.warp(block.timestamp + 13);

  // cannot offboard if quorum is not met
  vm.expectRevert('LendingTermOffboarding: quorum not met');
  offboarder.offboard(address(term));

  // prepare (2)
  offboarder.supportOffboard(snapshotBlock, address(term));
  assertEq(offboarder.canOffboard(address(term)), true);

  // properly offboard a term
  assertEq(guild.isGauge(address(term)), true);
  assertEq(psm.redemptionsPaused(), false);
  assertEq(offboarder.nOffboardingsInProgress(), 0);
  offboarder.offboard(address(term));
  assertEq(guild.isGauge(address(term)), false);
  assertEq(psm.redemptionsPaused(), true);
  assertEq(offboarder.nOffboardingsInProgress(), 1);

  // get enough CREDIT to pack back interests
  vm.stopPrank();
  vm.roll(block.number + 1);
  vm.warp(block.timestamp + 13);
  uint256 debt = term.getLoanDebt(aliceLoanId);
  credit.mint(alice, debt - aliceLoanSize);

  vm.startPrank(alice);
  // can close loans
  credit.approve(address(term), debt);
  term.repay(aliceLoanId);

  // cannot open new loans
  collateral.approve(address(term), aliceLoanSize);
  vm.expectRevert('LendingTerm: debt ceiling reached');
  aliceLoanId = term.borrow(aliceLoanSize, aliceLoanSize);
  vm.stopPrank();

  // do onboard another time
  vm.startPrank(bob);
  proposalId = onboarder.proposeOnboard(address(term));
  (targets, values, calldatas, description) = onboarder.getOnboardProposeArgs(address(term));
  vm.roll(block.number + 1);
  vm.warp(block.timestamp + 13);
  onboarder.castVote(proposalId, uint8(GovernorCountingSimple.VoteType.For));
  vm.roll(block.number + _VOTING_PERIOD + 1);
  vm.warp(block.timestamp + 13);
  onboarder.queue(targets, values, calldatas, keccak256(bytes(description)));
  vm.roll(block.number + 1);
  vm.warp(block.timestamp + _TIMELOCK_MIN_DELAY + 13);
  onboarder.execute(targets, values, calldatas, keccak256(bytes(description)));
  vm.stopPrank();

  // check execution
  assertEq(guild.isGauge(address(term)), true);

  // withdraw still paused after re-onboarding and nOffboardingsInProgress stays above 0
  assertEq(psm.redemptionsPaused(), true);
  assertEq(offboarder.nOffboardingsInProgress(), 1);

  // cleanup call and therefore unpause not possible:
  vm.expectRevert('LendingTermOffboarding: re-onboarded');
  offboarder.cleanup(address(term));
}

Impact

The offboarding redemption potentially stays paused for a longer time as a manual governance process is needed to unpause them, therefore a temporary DoS. And the pause / unpause mechanism of the offboarding process stays broken forever as the nOffboardingsInProgress variable will always be above 0.

Recommendations

Update the setRedemptionsPaused and nOffboardingsInProgress parameters on re-onboarding, or do not allow re-onboarding before the cleanup is done.

Assessed type

Context

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1147

[c4-judge]

Trumpero marked the issue as duplicate of #1141

[c4-judge]

Trumpero marked the issue as satisfactory