Ethereum Credit Guild - 0xdice91's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L216-L290 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L228-L231 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L274-L284

Vulnerability details

In SurplusGuildMinter.sol getRewards() is used to get rewards from a staking position, the variable userStake is first declared to reserve memory to hold a UserStake struct when the function is called, but the variable has not been initialised yet. The variable is only initialised further down in the function

userStake = _stakes[user][term];

The issue here is that the variable userStake is used to determine if the user has been slashed or not before it has been initialised leading to its components being in their default values

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

As userStake.lastGaugeLoss still has its default value of 0, if the term has ever suffered a loss in the past even before the user staked, the check will always return true (that is slashed will be set to true).

Impact

As a result of this error, Users guildReward and their staked first loss capital (credit token) will be unfairly slashed and lost forever whenever getRewards() is called.

            if (slashed) {
                guildReward = 0;
            }

        if (slashed) {
            emit Unstake(block.timestamp, term, uint256(userStake.credit));
            userStake = UserStake({
                stakeTime: uint48(0),
                lastGaugeLoss: uint48(0),
                profitIndex: uint160(0),
                credit: uint128(0),
                guild: uint128(0)
            });
            updateState = true;
        }

Tools Used

Manual Review

Recommended Mitigation Steps

The UserStake memory userStake variable should simply be initialised first with the user values before the check for if the user has been slashed is done.

    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]; // The initialisation should be moved there before the check for if slashed 
        lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
            slashed = true;
        }


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

//More code

}

Assessed type

Invalid Validation