Ethereum Credit Guild - cats's results

Lines of code

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

Vulnerability details

Impact

Users are not able to unstake their tokens or receive rewards for them when they should be able to.

Proof of Concept

Due to not caching the userStake info inside the function SurplusGuildMinter::getRewards before making a critical comparison, whenever a gauge takes a single loss, all user rewards will be slashed.

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 comparison here
            slashed = true;
        }

        // if the user is not staking, do nothing
        userStake = _stakes[user][term]; // @audit caching here

userStake.lastGaugeLoss will always be the default value of uint256 which is 0. Whenever a gauge takes a single loss, this comparison will always be true, thus slashed will be true.

Furthermore, since the getRewards function is called every time when a user tries to unstake, as long as a gauge takes a single loss, all users that have staked tokens will not be able to unstake them even if they should be able to. Clear loss of funds/tokens for the users.

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;

Tools Used

Manual Review

Recommended Mitigation Steps

Cache the userStake before the comparison.

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;
+       userStake = _stakes[user][term];
        lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) { // @audit comparison here
            slashed = true;
        }

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

Assessed type

Invalid Validation