Summary
The staking pool contract allows for adding new fees, but it fails to update the rewards before adding additional fees, resulting in higher-than-expected fees being taken from existing rewards. This causes overcharging of fees on pending rewards
Vulnerability Details
The issue arises in the `addFee` function, where new fees can be added by the contract owner without properly updating the reward calculations. This oversight causes the new fee to be applied retroactively to pending rewards, leading to incorrect fee deductions.
### Staking Pool Implementation:
* @notice Adds a new fee
* @param _receiver receiver of fee
* @param _feeBasisPoints fee in basis points
**/
function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {]
@audit>>1 . >> fees.push(Fee(_receiver, _feeBasisPoints));
require(_totalFeesBasisPoints() <= 4000, "Total fees must be <= 40%");
}
In the above implementation, the contract allows for adding new fees without updating the strategy rewards. As a result, the new fee is calculated alongside old rewards without adjusting for the rewards that have already accumulated. This can lead to overcharging and reduces the accuracy of the reward system.
By contrast, the `VaultController` strategy takes care of this by updating the strategy rewards before adding a new fee:
* @notice Adds a new fee
* @dev stakingPool.updateStrategyRewards is called to credit all past fees at
* the old rate before the percentage changes
* @param _receiver receiver of fee
* @param _feeBasisPoints fee in basis points
**/
function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {
@audit>>2. >> _updateStrategyRewards();
fees.push(Fee(_receiver, _feeBasisPoints));
if (_totalFeesBasisPoints() > 3000) revert FeesTooLarge();
}
Here, the `_updateStrategyRewards` function ensures that any rewards accumulated up to that point are properly distributed using the current fee rates before any new fees are added. This avoids retroactive changes to previously accrued rewards.
* @notice Distributes rewards/fees based on balance changes in strategies since the last update
* @param _strategyIdxs indexes of strategies to update rewards for
* @param _data update data passed to each strategy
**/
function _updateStrategyRewards(uint256[] memory _strategyIdxs, bytes memory _data) private {
int256 totalRewards;
uint256 totalFeeAmounts;
uint256 totalFeeCount;
address[][] memory receivers = new address[][]();
uint256[][] memory feeAmounts = new uint256[][]();
for (uint256 i = 0; i < _strategyIdxs.length; ++i) {
IStrategy strategy = IStrategy(strategies[_strategyIdxs[i]]);
(
int256 depositChange,
address[] memory strategyReceivers,
uint256[] memory strategyFeeAmounts
) = strategy.updateDeposits(_data);
totalRewards += depositChange;
if (strategyReceivers.length != 0) {
receivers[i] = strategyReceivers;
feeAmounts[i] = strategyFeeAmounts;
totalFeeCount += receivers[i].length;
for (uint256 j = 0; j < strategyReceivers.length; ++j) {
totalFeeAmounts += strategyFeeAmounts[j];
}
}
}
if (totalRewards != 0) {
totalStaked = uint256(int256(totalStaked) + totalRewards);
}
if (totalRewards > 0) {
receivers[receivers.length - 1] = new address[]();
feeAmounts[feeAmounts.length - 1] = new uint256[]();
totalFeeCount += fees.length;
for (uint256 i = 0; i < fees.length; i++) {
@audit>>3 . >> receivers[receivers.length - 1][i] = fees[i].receiver;
@audit>>4 . >> feeAmounts[feeAmounts.length - 1][i] =
(uint256(totalRewards) * fees[i].basisPoints) /
10000;
totalFeeAmounts += feeAmounts[feeAmounts.length - 1][i];
}
}
@audit>>5 . >> if (totalFeeAmounts >= totalStaked) {
totalFeeAmounts = 0;
}
@audit>>6 . >> if (totalFeeAmounts > 0) {
@audit>>7 . >> uint256 sharesToMint = (totalFeeAmounts * totalShares) /
(totalStaked - totalFeeAmounts);
_mintShares(address(this), sharesToMint);
uint256 feesPaidCount;
for (uint256 i = 0; i < receivers.length; i++) {
for (uint256 j = 0; j < receivers[i].length; j++) {
if (feesPaidCount == totalFeeCount - 1) {
transferAndCallFrom(
address(this),
receivers[i][j],
balanceOf(address(this)),
"0x"
);
} else {
transferAndCallFrom(address(this), receivers[i][j], feeAmounts[i][j], "0x");
feesPaidCount++;
}
}
}
}
Impact
- **Incorrect Fee Calculation**: The staking pool will apply new fees to the total pending rewards, which may include rewards that should have been calculated using the old fee structure.
- **Overcharging**: As a result, the protocol will charge more fees than intended
Tools Used
Manual Review
Recommendations
To mitigate this issue, the contract should update the strategy rewards before any new fees are added. This ensures that pending rewards are credited using the current fee rate, preventing the new fee from being applied retroactively.
### Suggested Solution:
Add a call to `_updateStrategyRewards` before pushing new fees in the staking pool contract:
function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {
++ _updateStrategyRewards();
fees.push(Fee(_receiver, _feeBasisPoints));
require(_totalFeesBasisPoints() <= 4000, "Total fees must be <= 40%");
}
