Liquid Staking

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

Incorrect Fee Update in LSTRewardsSplitter Allows Temporary Exceeding of Total Fee Limit

Summary

updateFee function has a vulnerability that allows the total fees to temporarily exceed the limit of 10000 basis points (100%) before reverting. This occurs because the function updates the fee in storage before checking if the total fees exceed the limit.

  • The updateFee function allows the owner to update an existing fee at a given index.

  • It first checks if the provided _index is valid and the fee exists.

  • If the new _feeBasisPoints is 0, it removes the fee by replacing it with the last fee in the array and popping the last element.

  • Otherwise, it updates the receiver and basisPoints of the fee at the given _index.

  • After updating the fee, it checks if the total fees basis points exceed the limit of 10000.

  • If the total fees exceed the limit, it reverts with the FeesExceedLimit error.

Vulnerability Details

The issue arises because the function first updates the fee at the specified index and then checks if the total fees exceed the limit of 10000 basis points (100%). This sequence of operations allows for a scenario where the total fees can temporarily exceed the limit before the function reverts with the FeesExceedLimit error.

The updateFee function: 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; // <-- Vuln: Fee update occurs here
fees[_index].basisPoints = _feeBasisPoints; // <-- Vuln: Fee update occurs here
}
if (_totalFeesBasisPoints() > 10000) revert FeesExceedLimit(); // <-- Vuln: Check is performed after the fee update
}

In the order of operations within the updateFee function

  1. The function updates the receiver and basisPoints of the fee at the given _index (lines 162-163).

  2. After updating the fee, it checks if the total fees basis points exceed the limit of 10000 (line 166).

The check if (_totalFeesBasisPoints() > 10000) revert FeesExceedLimit(); is performed after the fee has already been updated in storage. This allows for a temporary inconsistent state where the total fees can exceed 100% before the revert occurs.

  1. Let's say the LSTRewardsSplitter contract is deployed with an initial set of fees that add up to less than 10000 basis points (e.g., 9000 basis points).

  2. Call the updateFee function with an _index that exists and a _feeBasisPoints value that, when added to the existing fees, exceeds 10000 (e.g., 2000 basis points).

  3. The function will update the fee at the given index, and then revert with the FeesExceedLimit error.

  4. However, the fee update would have already been applied to the contract's state before the revert occurs, resulting in a temporary inconsistent state.

Impact

The contract's state can be in an inconsistent state where the total fees exceed 100% before the revert occurs.

Tools Used

Vs Code

Recommendations

Perform the _totalFeesBasisPoints check before updating the fee in storage.

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 about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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