Users can prevent slashing by frontrunning gauge loss notifications

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

The getRewards() function in SurplusGuildMinter checks if a user is slashable by obtaining the gauge’s lastGaugeLoss() (a value storing the timestamp of the latest loss notification in the gauge) and comparing it against the staked user’s last gauge loss stored timestamp. Such value is updated for the gauge when notifyPnL() is called, which internally will trigger GuildToken's notifyGaugeLoss(), which will set the current timestamp as the lastGaugeLoss for the gauge:

// ProfitManager.sol
function notifyPnL( 
        address gauge,
        int256 amount
    ) external onlyCoreRole(CoreRoles.GAUGE_PNL_NOTIFIER) {
        uint256 _surplusBuffer = surplusBuffer;
        uint256 _termSurplusBuffer = termSurplusBuffer[gauge];
        address _credit = credit;

        // handling loss
        if (amount < 0) {
            uint256 loss = uint256(-amount);

            // save gauge loss
            GuildToken(guild).notifyGaugeLoss(gauge);

            ...
        }

			 // handling profit
       ...
    }

// GuildToken.sol
function notifyGaugeLoss(address gauge) external {
	require(msg.sender == profitManager, "UNAUTHORIZED");
	
	// save gauge loss
	lastGaugeLoss[gauge] = block.timestamp;
	emit GaugeLoss(gauge, block.timestamp); 
}

In order to determine if a user is slashable, lastGaugeLoss must be greater than userStake.lastGaugeLoss . This check actually verifies if the user was staked in the gauge prior or during the loss notification, which makes him slashable.

// SurplusGuildMinter.sol
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;
        }

	...

}

Considering the previous information, a vulnerability regarding avoiding slashing arises. It is possible for a user to frontrun the notifyPnL() function execution when a negative PnL is reported in order to avoid being slashed. This is because no mechanism nor restriction exists in order to prevent such situation, which places the protocol in an undesirable and critical situation. Essentially, users will be able to stake to malicious/lossy gauges and obtain rewards without any real risk of being slashed because slash events can always be avoided.

It is important to highlight the fact that this is not just a regular issue where the protocol will lose money. The ECG protocol’s concept of trust minimized pooled lending, and the staking mechanism built for it particularly, rely on the importance of “skin in the game”, i.e. users risking their assets by staking in well-functioning gauges for the proper functioning of the protocol. ECG’S oracle-less architecture is made possible based on the trust assumption that users (and particularly stakers) will not misbehave by voting for maliciously onboarded lending terms due to a punishment being imposed on them (in this particular case, the slashing). If the punishment (slashing) is avoided, then the whole ECG mechanisms will be broken, preventing the protocol from properly meeting its functioning requirements and expectations.

Tools Used

Manual review

Recommended Mitigation Steps

Usually, staking contracts incorporate restrictions and mitigation mechanisms for situations such as the one described previously. I recommend rethinking the staking approach, incorporating ideas from third-party protocols that already incorporate properly-working staking mechanisms, such as Chainlink Staking V0.2 or synthetix staking rewards.

A good first addition could be adding some sort of locking period that guarantees that users will stake their tokens for a minimum amount of time. Later, some additions can be added, such as unbonding mechanisms or periods.

Assessed type

MEV

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #906

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #877

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid