Summary
The staking pool contract allows for updating new fees, but it fails to update the rewards before fees reduction/increase, resulting in lower/higher-than-expected fees being taken from existing rewards. This causes overcharging/undercharging of fees on pending rewards
Vulnerability Details
The issue arises in the `Updatefee` function, where updates of fees can be done by the contract owner without properly updating the reward calculations. This oversight causes the updated fee to be applied retroactively to pending rewards, leading to incorrect fee deductions.
### Staking Pool Implementation:
* @notice Updates an existing fee
* @param _index index of fee
* @param _receiver receiver of fee
* @param _feeBasisPoints fee in basis points
**/
@audit>>1. >> function updateFee(
uint256 _index,
address _receiver,
uint256 _feeBasisPoints
) external onlyOwner {
@audit>>2. no update >>
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;
}
require(_totalFeesBasisPoints() <= 4000, "Total fees must be <= 40%");
}
In the above implementation, the contract allows for updating new fees without updating the strategy rewards. As a result, q new fee structure is used for calculations alongside old rewards without adjusting for the rewards that have already accumulated. This can lead to undercharging/overcharging and reduces the accuracy of the reward system.
By contrast, the `VaultController` strategy takes care of this by updating the strategy rewards before updating a new fee:
* @notice Updates an existing fee
* @dev stakingPool.updateStrategyRewards is called to credit all past fees at
* the old rate before the percentage changes
* @param _index index of fee
* @param _receiver receiver of fee
* @param _feeBasisPoints fee in basis points
**/
function updateFee(
uint256 _index,
address _receiver,
uint256 _feeBasisPoints
) external onlyOwner {
@audit>>2. >> _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();
}
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/Undercharging**: As a result, the protocol will charge more/less 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:
* @notice Updates an existing fee
* @param _index index of fee
* @param _receiver receiver of fee
* @param _feeBasisPoints fee in basis points
**/
function updateFee(
uint256 _index,
address _receiver,
uint256 _feeBasisPoints
) external onlyOwner {
++ _updateStrategyRewards();
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;
}
require(_totalFeesBasisPoints() <= 4000, "Total fees must be <= 40%");
}
