Ethereum Credit Guild - Akali's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L229-L231

Vulnerability details

Impact

After a lending term (gauge) incurs a loss, the SurplusGuildMinter.getRewards() function always returns slashed = true, even when this term is deprecated and then onboarded again. This results in users losing their assets if they stake in this term afterward because they will always be slashed in this term.

Proof of Concept

In the SurplusGuildMinter.getRewards function, userStake is a new memory struct with userStake.lastGaugeLoss set to 0. Therefore, if this term incurred a loss in the past, GuildToken(guild).lastGaugeLoss(term) always returns a non-zero value. Consequently, lastGaugeLoss will be greater than userStake.lastGaugeLoss, causing slashed to always be true.

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

Additionally, even when a term is deprecated, the lastGaugeLoss of this term is still saved in the storage of the GuildToken contract. Therefore, it will always return a non-zero value, and users who stake in a re-added lending term will always be slashed.

Tools Used

Manual review

Recommended Mitigation Steps

getRewards function should load from _stakes[user][term] before validation of slashed variable.

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

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L141

Vulnerability details

Impact

The SurplusGuildMinter.claimRewards function loops and claims all rewards from active lending terms delegated by this contract. Consequently, this function can run out of gas and revert, potentially causing a DoS state on other functions of SurplusGuildMinter. This situation may occur when the market has numerous lending terms, and users stake in many of them.

Proof of Concept

In the SurplusGuildMinter contract, getRewards function is used to claim and transfer rewards from a lending term (gauge) to the user. However, it calls the claimRewards function from the ProfitManager contract, which loops and claims rewards from all delegated lending terms of this contract. That action is unnecessary because it only needs to claim the reward of a specific term, resulting in the risk of reverting due to running out of gas for this function.

function getRewards(
    address user,
    address term
)...
{
    ...
    ProfitManager(profitManager).claimRewards(address(this));
    ...
}

function claimRewards(
    address user
) external returns (uint256 creditEarned) {
    address[] memory gauges = GuildToken(guild).userGauges(user);
    for (uint256 i = 0; i < gauges.length; ) {
        creditEarned += claimGaugeRewards(user, gauges[i]);
        unchecked {
            ++i;
        }
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

getRewards function should call claimGaugeRewards function instead of claimRewards:

ProfitManager(profitManager).claimGaugeRewards(address(this), term);

Assessed type

DoS