Ethereum Credit Guild - XDZIBECX's results

Lines of code

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

Vulnerability details

Impact

in the the getRewards function user slashing logic is executed based on an uninitialized UserStake struct. As a result, users may be falsely considered slashed before any actual loss, erroneously affecting staking rewards and other related functionalities. and that cause the incorrect variable initialization , the The variable userStake is used to compare with lastGaugeLoss before actually fetching the UserStake struct from _stakes[user][term] and This causes the comparison if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) to use a default-initialized UserStake object, which has the lastGaugeLoss field default to 0 and this problem can result in all users being considered slashed (slashed = true), as lastGaugeLoss is usually non-zero (once any gauge loss occurs) and will always be greater than the default 0 value of an uninitialized userStake.lastGaugeLoss. This could lead to incorrect logic where users are considered slashed incorrectly, affecting the functionality of reward distribution or other contract behaviors. here is the vulenrbale part :

 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)) {
            slashed = true;
        }

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

        // compute CREDIT rewards
        ProfitManager(profitManager).claimRewards(address(this)); // this will update profit indexes
        uint256 _profitIndex = ProfitManager(profitManager)
            .userGaugeProfitIndex(address(this), term);
        uint256 _userProfitIndex = uint256(userStake.profitIndex);

        if (_profitIndex == 0) _profitIndex = 1e18;
        if (_userProfitIndex == 0) _userProfitIndex = 1e18;

        uint256 deltaIndex = _profitIndex - _userProfitIndex;

        if (deltaIndex != 0) {
            uint256 creditReward = (uint256(userStake.guild) * deltaIndex) /
                1e18;
            uint256 guildReward = (creditReward * rewardRatio) / 1e18;
            if (slashed) {
                guildReward = 0;
            }

            // forward rewards to user
            if (guildReward != 0) {
                RateLimitedMinter(rlgm).mint(user, guildReward);
                emit GuildReward(block.timestamp, user, guildReward);
            }
            if (creditReward != 0) {
                CreditToken(credit).transfer(user, creditReward);
            }

            // save the updated profitIndex
            userStake.profitIndex = SafeCastLib.safeCastTo160(_profitIndex);
            updateState = true;
        }

        // if a loss occurred while the user was staking, the GuildToken.applyGaugeLoss(address(this))
        // can be called by anyone to slash address(this) and decrement gauge weight etc.
        // The contribution to the surplus buffer is also forfeited.
        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;
        }

        // store the updated stake, if needed
        if (updateState) {
            _stakes[user][term] = userStake;
        }
    }

Proof of Concept

• the function body, lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term); is called before userStake is initialized from state using userStake = _stakes[user][term];. This causes the uninitialized userStake to be used in a comparison.
• Since userStake is a memory struct that has not yet been assigned any value, its fields, including lastGaugeLoss, will default to zero in Solidity.
• The line if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) { slashed = true; } is comparing lastGaugeLoss, which could be any non-zero value received from GuildToken(guild).lastGaugeLoss(term), with the uninitialized (zeroed) userStake.lastGaugeLoss. This results in a slashed condition being set to true in any case where lastGaugeLoss is a non-zero value. and let's say r a scenario where lastGaugeLoss from GuildToken returns a non-zero timestamp of a loss event. The uninitialized userStake has a default lastGaugeLoss of zero. The condition lastGaugeLoss > uint256(userStake.lastGaugeLoss) would evaluate to true every time, thus making it impossible for anyone to claim rewards correctly because slashed would always be set to true.

Tools Used

• manual review

Recommended Mitigation Steps

need to Retrieve the userStake data from contract storage before performing any logic comparisons as :

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

Assessed type

Other