c4-bot-9 commented 6 months ago

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L158-L212

Vulnerability details

Proof of Concept

Take a look at SurplusGuildMinter.sol#L158-L21

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

        // if the user has been slashed, there is nothing to do
        if (slashed) return;

        // check that the user is at least staking `amount` CREDIT
        require(
            amount != 0 && userStake.credit >= amount,
            "SurplusGuildMinter: invalid amount"
        );

        // update stake
        uint256 userMintRatio = (uint256(userStake.guild) * 1e18) /
            userStake.credit; /// upcast guild to prevent overflow
        uint256 guildAmount = (userMintRatio * amount) / 1e18;

        if (amount == userStake.credit) guildAmount = userStake.guild;

        userStake.credit -= SafeCastLib.safeCastTo128(amount);
        userStake.guild -= SafeCastLib.safeCastTo128(guildAmount);

        if (userStake.credit == 0) {
            userStake.stakeTime = 0;
            userStake.lastGaugeLoss = 0;
            userStake.profitIndex = 0;
        } else {
            // if not unstaking all, make sure the stake remains
            // greater than the minimum stake
            require(
                userStake.credit >= MIN_STAKE,
                "SurplusGuildMinter: remaining stake below min"
            );
        }
        _stakes[msg.sender][term] = userStake;

        // withdraw & transfer CREDIT
        //@audit-issue
        ProfitManager(profitManager).withdrawFromTermSurplusBuffer(
            term,
            msg.sender,
            amount
        );

        // burn GUILD
        GuildToken(guild).decrementGauge(term, guildAmount);
        RateLimitedMinter(rlgm).replenishBuffer(guildAmount);
        GuildToken(guild).burn(guildAmount);

        // emit event
        emit Unstake(block.timestamp, term, amount);
    }

As seen, this function is used in order for users to unstake their CREDIT tokens and even stop voting in a gauge, issue is with the call this function makes to the ProfitManager.withdrawFromTermSurplusBuffer(), note that this is unlike the stake() function that queries ProfitManager.donateToTermSurplusBuffer(), cause whereas donateToTermSurplusBuffer() is accessible to all, withdrawFromTermSurplusBuffer() is restricted to the GUILD_SURPLUS_BUFFER_WITHDRAW admin alone and as such a user's attempt to unstake would fail since they don't have this role, the restriction of withdrawFromTermSurplusBuffer() can be proven by adding the test below to the ProfitManager.t.sol

function testUnauthorozedWithdrawFromTermSurplusBuffer() public {
    // Initial state setup:
    // Mint 1000e18 (1000 * 10^18) units of the credit token to this contract.
    credit.mint(address(this), 1000e18);

    // Approve the ProfitManager contract to spend 1000e18 units on behalf of this contract.
    credit.approve(address(profitManager), 1000e18);

    // Donate 1000e18 units to the term surplus buffer of this contract's address.
    profitManager.donateToTermSurplusBuffer(address(this), 1000e18);

    // Verify that the term surplus buffer for this contract's address is correctly set to 1000e18.
    assertEq(profitManager.termSurplusBuffer(address(this)), 1000e18);

    // Verify that the balance of this contract is now 1000e18 units,
    // reflecting the minted amount.
    assertEq(credit.balanceOf(address(this)), 1000e18);

    // Verify that the balance of the ProfitManager contract is 1000e18 units,
    // reflecting the donated amount.
    assertEq(credit.balanceOf(address(profitManager)), 1000e18);

    // Attempt unauthorized withdrawal:
    // Expect a revert due to lack of authorization (UNAUTHORIZED) when trying to withdraw from the surplus buffer.
    vm.expectRevert("UNAUTHORIZED");
    profitManager.withdrawFromTermSurplusBuffer(address(this), address(this), 100e18);

    // Granting withdrawal role:
    // Temporarily assume the role of the governor to grant the withdrawal role to this contract.
    vm.prank(governor);
    core.grantRole(CoreRoles.GUILD_SURPLUS_BUFFER_WITHDRAW, address(this));

    // Authorized withdrawal:
    // Perform an authorized withdrawal of 100e18 units from the term surplus buffer.
    profitManager.withdrawFromTermSurplusBuffer(address(this), address(this), 100e18);

    // Verify that the term surplus buffer for this contract's address is now reduced by the withdrawn amount (100e18) and is equal to 900e18
    assertEq(profitManager.termSurplusBuffer(address(this)), 900e18);
}

Impact

Dos from unstaking, breaking contract's intended logic since users can no longer stop voting in a guage, additionally this could lead to a loss in US$ value if users decide they want to sell their CREDIT tokens but can't do so and the price of this asset flash drops (highly possible, being that the bull run is kicking in)

Recommended Mitigation Steps

Reimplement the unstaking logic, and allow users to be able to stop voting in a guage if they decide to.

Assessed type

Access Control

c4-pre-sort commented 5 months ago

0xSorryNotSorry marked the issue as sufficient quality report

c4-pre-sort commented 5 months ago

0xSorryNotSorry marked the issue as duplicate of #764

c4-judge commented 5 months ago

Trumpero changed the severity to QA (Quality Assurance)

c4-judge commented 4 months ago

Trumpero marked the issue as grade-b

code-423n4 / 2023-12-ethereumcreditguild-findings