Ethereum Credit Guild - 0xmystery's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L175-L199 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L116-L148

Vulnerability details

Impact

The identified issue lies in supportOffboard() of the LendingTermOffboarding smart contract, which lacks mechanisms to prevent GUILD holders from voting on a lending term that has already been offboarded and cleaned up. This contradicts the function NatSpec of offboard() that is known to be saying,

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L151

    /// This will prevent new loans from being open, and will prevent GUILD holders to vote for the term.

As can been seen in the code logic of supportOffboard() , any user who has not voted on an unexpired poll can support the poll even though (_weight + userWeight >= quorum) has been fulfilled:

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L115-L148

    /// @notice Support a poll to offboard a given LendingTerm.
    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;
        if (_weight + userWeight >= quorum) {
            canOffboard[term] = true;
        }
        emit OffboardSupport(
            block.timestamp,
            term,
            snapshotBlock,
            msg.sender,
            userWeight
        );
    }

This oversight could lead to the reactivation of the canOffboard[term] flag for terms that have completed the offboarding process where an exploit could be crafted to nullify the mutext logic of cleanup(). The primary impact of this issue is unpausing redemptions in SimplePSM.sol pre-maturely while other terms that have been offboarded are yet to have their loans closed.

Proof of Concept

Here's a scenario:

1. Term 1, 2, and 3 each goes through proposeOffboard(), and then supportOffboard() till quorum is met to make canOffboard[term] = true:

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L138-L140

        if (_weight + userWeight >= quorum) {
            canOffboard[term] = true;
        }

2. Next, offboard(() is separately called to post-increment nOffboardingsInProgress to 3 where the first call will have SimplePSM.redeem() paused:

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L161-L167

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

3. Term 1 has all of its loans closed and successfully executes cleanup() to pre-decrement nOffboardingsInProgress to 2, but understandably not meeting the following condition to unpause redemptions in SimplePSM.sol yet. And, at this point, canOffboard[term] is set to false for term 1:

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L190-L195

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

        canOffboard[term] = false;

4. However, a user who has not voted for term 1 manages to call supportOffboard() to turn on canOffboard[term] to true again and then calling cleanup() to get through the first require check, supposedly a mutex lock for a term that has already been cleaned up:

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L172-L199

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

5. The second and the third require checks also pass. Likewise, protocol config on core().revokeRole() also get through as they only return bool, i.e. false in this case and don't revert on a role that has already been revoked:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L193-L208

    /**
     * @dev Attempts to revoke `role` to `account` and returns a boolean indicating if `role` was revoked.
     *
     * Internal function without access restriction.
     *
     * May emit a {RoleRevoked} event.
     */
    function _revokeRole(bytes32 role, address account) internal virtual returns (bool) {
        if (hasRole(role, account)) {
            _roles[role].hasRole[account] = false;
            emit RoleRevoked(role, account, _msgSender());
            return true;
        } else {
            return false;
        }
    }

6. nOffboardingsInProgress is pre-decremented to 1, but steps 4. and 5. can be repeated by another voter who has not voted to eventually unpause psm redemptions against the intended design while terms 2 and 3 are yet to be cleaned up:

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SimplePSM.sol#L132-L154

    /// @notice redeem `amountIn` CREDIT for `amountOut` underlying tokens and send to address `to`
    /// @dev see getRedeemAmountOut() to pre-calculate amount out
    function redeem(
        address to,
        uint256 amountIn
    ) external returns (uint256 amountOut) {
        require(!redemptionsPaused, "SimplePSM: redemptions paused");
        amountOut = getRedeemAmountOut(amountIn);
        CreditToken(credit).burnFrom(msg.sender, amountIn);
        pegTokenBalance -= amountOut;
        ERC20(pegToken).safeTransfer(to, amountOut);
        emit Redeem(block.timestamp, to, amountIn, amountOut);
    }

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

Tools Used

Manual

Recommended Mitigation Steps

Consider the following refactoring steps. The primary goal is to ensure that once a lending term is offboarded and cleaned up, it cannot be voted on again in supportOffboard() :

1. Status Tracking for Terms Introduce a new status enum and variable to track the offboarding status of each term.

enum OffboardingStatus { Active, Offboarded, CleanedUp }

mapping(address => OffboardingStatus) public termStatus;

2. Modify supportOffboard() Logic Amend supportOffboard() to check the term's offboarding status. If a term has been cleaned up, it should not accept further votes. (Note: OffboardingStatus.Active is the default status when termStatus[term] remains uninitialized/updated.)

function supportOffboard(uint256 snapshotBlock, address term) external whenNotPaused {
    require(termStatus[term] == OffboardingStatus.Active, "Term already offboarded or cleaned up");
    // existing logic...
}

3. Update offboard() Logic In offboard(), set the status to Offboarded once a term is offboarded. (Note: This refactoring is optional since GuildToken(guildToken).removeGauge(term) is laready in place to prevent offboarding. However, it will will prevent GUILD holders to vote for the term again as commented by the function NatSpec:

function offboard(address term) external whenNotPaused {
    // existing offboarding logic...
    termStatus[term] = OffboardingStatus.Offboarded;
}

4. Update cleanup() Logic In cleanup(), set the status to CleanedUp after successful cleanup:

function cleanup(address term) external whenNotPaused {
    // existing cleanup checks and logic...
    termStatus[term] = OffboardingStatus.CleanedUp;
}

Assessed type

Access Control