Liquid Staking

Summary

https://github.com/Cyfrin/2024-09-stakelink/blob/main/contracts/core/rewardsPools/RewardsPool.sol

Location: Function _withdraw(address _account)

The _withdraw function is responsible for processing the withdrawal of earned rewards for a user. The current implementation directly subtracts the amount to withdraw from the user's total rewards without first ensuring that the user has enough rewards to cover the withdrawal.

'function _withdraw(address _account) internal virtual {
uint256 toWithdraw = withdrawableRewards(_account);
if (toWithdraw > 0) {
updateReward(_account);
userRewards[_account] -= toWithdraw; // Potential issue here
totalRewards -= toWithdraw;
token.safeTransfer(_account, toWithdraw);
emit Withdraw(_account, toWithdraw);
}
}'

Impact

This vulnerability can lead to several issues:

1. Incorrect Reward Calculation: If a user has 100 rewards and attempts to withdraw 150, although Solidity 0.8+ prevents underflows, the logical flow may lead to an unexpected state, causing the contract to misbehave in further operations.

2. User Frustration: Users may become frustrated if they find that their expected rewards are not accurately reflected, leading to a loss of trust in the contract and its underlying mechanisms.

3. Potential for Future Bugs: The lack of checks increases the risk of future bugs in related functions that rely on accurate user rewards.

Proof of Concept (PoC)

The following scenario illustrates how this vulnerability can be exploited:

1. A user deposits tokens into the rewards pool and earns rewards over time.

2. Assume the user has accumulated 100 rewards and wants to withdraw:

3. '''rewardsPool.withdraw(); // This will call _withdraw'''

4. If the user somehow attempts to withdraw an amount greater than their available rewards (for example, via a malicious or unintended call), the contract logic does not prevent the subtraction of the total rewards:

// User has 100 rewards
userRewards[msg.sender] = 100;
// User tries to withdraw 150 rewards
userRewards[msg.sender] -= 150; // This would normally revert but could lead to logical errors.

Tools Used

Manual Review

Recommendations

To mitigate this vulnerability, the following changes should be made:

1. Implement a Check Before Subtraction: Add a check to ensure that the user has sufficient rewards before allowing the withdrawal:

require(userRewards[_account] >= toWithdraw, "Insufficient rewards");

2.Modify the _withdraw Function: Update the function to include the check:

function _withdraw(address _account) internal virtual {
uint256 toWithdraw = withdrawableRewards(_account);
require(toWithdraw > 0, "No rewards to withdraw");
updateReward(_account);
require(userRewards[_account] >= toWithdraw, "Insufficient rewards");
userRewards[_account] -= toWithdraw;
totalRewards -= toWithdraw;
token.safeTransfer(_account, toWithdraw);
emit Withdraw(_account, toWithdraw);
}