Liquid Staking

Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Submission Details
Severity: medium
Invalid

Updating Fees in LSTRewardsSplitter Can Exceed Maximum Fee Limit

Summary

The updateFee function in the LSTRewardsSplitter contract has a vulnerability that allows updating a fee that causes the total fee basis points to exceed the maximum limit of 10000.

Vulnerability Details

The updateFee function takes the index, receiver address, and new fee basis points as parameters. The issue arises because the function updates the fee first and then checks if the total fee basis points exceed the maximum limit. This means that if the new fee basis points cause the total to exceed the limit, the fee will still be updated, and only afterwards will the function revert with the FeesExceedLimit error.

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol#L158-L167

if (_feeBasisPoints == 0) {
fees[_index] = fees[fees.length - 1];
fees.pop();
} else {
fees[_index].receiver = _receiver;
fees[_index].basisPoints = _feeBasisPoints;
}
if (_totalFeesBasisPoints() > 10000) revert FeesExceedLimit();

The issue is that the function first updates the fee at the given _index with the new _receiver and _feeBasisPoints values. Only after the fee is updated, it checks if the total fee basis points exceed the maximum limit of 10000.

This means that if the new _feeBasisPoints value causes the total fee basis points to exceed the limit, the fee will still be updated, and the FeesExceedLimit error will be thrown afterwards. However, at this point, the fee has already been modified, which is problematic.

Impact

If the total fee basis points exceed the maximum limit, the rewards distribution will not be proportional to the intended fee percentages. This can result in some fee receivers getting more or less rewards than expected.

If the updateFee function is called with parameters that cause the total fee basis points to exceed the limit, the transaction will revert.

Tools Used

Manual Review

Recommendations

Check if the new total fee basis points exceed the limit before updating the fee. If the limit is exceeded, the function should revert without modifying the fee. By calculating the new total fee basis points before updating the fee and checking against the limit, the contract ensures that the total fees remain within the allowed range, preventing the vulnerability.

function updateFee(
uint256 _index,
address _receiver,
uint256 _feeBasisPoints
) external onlyOwner {
require(_index < fees.length, "Fee does not exist");
+ uint256 newTotalFees = _totalFeesBasisPoints() - fees[_index].basisPoints + _feeBasisPoints;
+ if (newTotalFees > 10000) revert FeesExceedLimit();
if (_feeBasisPoints == 0) {
fees[_index] = fees[fees.length - 1];
fees.pop();
} else {
fees[_index].receiver = _receiver;
fees[_index].basisPoints = _feeBasisPoints;
}
- if (_totalFeesBasisPoints() > 10000) revert FeesExceedLimit();
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.