Submission Details
Severity:
`_totalFeesBasisPoints()` Can Exceed 10000, Causing `_splitRewards(uint256 _rewardsAmount)` to Withdraw More Funds Than `_rewardsAmount`
## Summary
In the `LSTRewardsSplitter` contract, every time a change in the fee array occurs, the contract checks whether `_totalFeesBasisPoints()` has exceeded 10000, for example in the `updateFee` or `addFee` functions. However, in the constructor where the initial fee array is set, there is no check to ensure that `_totalFeesBasisPoints()` hasn't exceeded 10000.
As a result, in `LSTRewardsSplitter::_splitRewards`, the contract iterates through the `fee` array and calculates the amount to transfer to each fee receiver according to the following equation:
```solidity
uint256 amount = (_rewardsAmount * fee.basisPoints) / 10000;
```
If `_totalFeesBasisPoints()` exceeds 10000, it will cause more funds to be transferred than `_rewardsAmount`.
## Vulnerability Details & Impact
There are three ways in the `LSTRewardsSplitter` contract that can affect the `fee` array:
1)
```solidity
constructor(address _lst, Fee[] memory _fees, address _owner) {
controller = ILSTRewardsSplitterController(msg.sender);
lst = IERC677(_lst);
for (uint256 i = 0; i < _fees.length; ++i) {
fees.push(_fees[i]);
}
_transferOwnership(_owner);
}
```
2)
```solidity
function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {
fees.push(Fee(_receiver, _feeBasisPoints));
if (_totalFeesBasisPoints() > 10000) revert FeesExceedLimit();
}
```
3)
```solidity
function updateFee(uint256 _index, address _receiver, uint256 _feeBasisPoints) external onlyOwner {
require(_index < fees.length, "Fee does not exist");
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();
}
```
In the second and third examples (which also have the `onlyOwner` modifier), there is a safeguard:
```solidity
if (_totalFeesBasisPoints() > 10000) revert FeesExceedLimit();
```
This checks whether `_totalFeesBasisPoints()` exceeds 10000. However, in the constructor (1), there is no such check to ensure that `_totalFeesBasisPoints()` hasn't exceeded 10000 when the initial fee array is set.
If the total basis points exceed 10000, then in `LSTRewardsSplitter::_splitRewards`, the total amount transferred will be calculated as:
```solidity
_rewardsAmount * _totalFeesBasisPoints() / 10000
```
This total amount will be greater than `_rewardsAmount`, causing fewer funds to be added to `principalDeposits`. In some cases, this could even lead to a failure of the reward split because the function distributes more than the `_rewardsAmount` passed to `_splitRewards`.
**Note:** Similar to the `_totalFeesBasisPoints()` function in the `StakingPool` contract, there is a function by the same name that applies a similar check but on a different field. However, the initializer function in the `StakingPool` contract is not vulnerable because it includes the following requirement:
```solidity
require(_totalFeesBasisPoints() <= 4000, "Total fees must be <= 40%");
```
This ensures that the total fees do not exceed 40%.
## Tools Used
Manual review.
## Recommendations
Add a check in the constructor to ensure that `_totalFeesBasisPoints()` does not exceed 10000.
```diff
constructor(address _lst, Fee[] memory _fees, address _owner) {
controller = ILSTRewardsSplitterController(msg.sender);
lst = IERC677(_lst);
for (uint256 i = 0; i < _fees.length; ++i) {
fees.push(_fees[i]);
}
+ if (_totalFeesBasisPoints() > 10000) revert FeesExceedLimit();
_transferOwnership(_owner);
}
```
Prize pool breakdown
Total prize
50,000 USDC
nSLOC
2,53020 USDC / LOC
42,000 USDC
4,250 USDC
3,750 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.