Submission Details
Severity:
Lack of Reward Splitting in `splitRewards::addFee` Allows Past Rewards to Be Allocated to New Fee Receivers and Enables Front-Running by Current Fee Receivers
## Summary
In the `splitRewards::addFee` function, which is used to add a new fee receiver, there is no mechanism to first distribute the rewards to the current fee receivers. As a result, the newly added fee receiver will receive a portion of the current rewards, which should entirely belong to the previous fee receivers who have been staking or participating in other profitable activities. Additionally, there is a potential for front-running by the current fee receivers in both the `splitRewards::addFee` and `splitRewards::updateFee` functions (in cases where the `updateFee` function increases a user's `basisPoints`).
It is important to note that the `splitRewards::addFee` and `splitRewards::updateFee` functions also exist in the `VaultControllerStrategy::addFee` and `VaultControllerStrategy::updateFee` functions. However, these functions do not have this vulnerability because they contain the following logic before modifying the fee array:
```javascript
_updateStrategyRewards();
```
## Vulnerability Details & Impact
Consider the following two functions in the `splitRewards.sol` contract:
```javascript
function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {
fees.push(Fee(_receiver, _feeBasisPoints));
if (_totalFeesBasisPoints() > 10000) revert FeesExceedLimit();
}
```
```javascript
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();
}
```
Now compare this with the equivalent functions in the `VaultControllerStrategy.sol` contract:
```javascript
function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {
@> _updateStrategyRewards();
fees.push(Fee(_receiver, _feeBasisPoints));
if (_totalFeesBasisPoints() > 3000) revert FeesTooLarge();
}
```
```javascript
function updateFee(
uint256 _index,
address _receiver,
uint256 _feeBasisPoints
) external onlyOwner {
@> _updateStrategyRewards();
if (_feeBasisPoints == 0) {
fees[_index] = fees[fees.length - 1];
fees.pop();
} else {
fees[_index].receiver = _receiver;
fees[_index].basisPoints = _feeBasisPoints;
}
if (_totalFeesBasisPoints() > 3000) revert FeesTooLarge();
}
```
In the `VaultControllerStrategy.sol` contract, the `_updateStrategyRewards()` function is called before modifying the fee array. This prevents front-running by the previous fee receivers and ensures that the current rewards are fully distributed to the previous fee receivers rather than being shared with newly added fee receivers.
## Tools Used
Manual review.
## Recommendations
```diff
function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {
+ this.splitRewards();
fees.push(Fee(_receiver, _feeBasisPoints));
if (_totalFeesBasisPoints() > 10000) revert FeesExceedLimit();
}
```
Since the `splitRewards()` function is external, we call it using `this.splitRewards()`. Alternatively, `_splitRewards()` can be used with additional logic to handle negative rewards, similar to the logic implemented in the `splitRewards()` [function](https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol#L118):
```javascript
int256 newRewards = int256(lst.balanceOf(address(this))) - int256(principalDeposits);
```
Updates
Appeal created
inallhonesty 12 months ago
Submission Judgement Published Assigned finding tags:
`updateStrategyRewards` is not called before adding & updating the fees
It should be called with try and catch to avoid DOS by receiver.
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.