Summary
Modifications to the fee configuration for a StakingPool can make inadvertently make retroactive claims on due rewards.
Vulnerability Details
Calling addFee or updateFee makes stepwise changes to the fee distribution model of the StakingPool which influence outstanding rewards.
Notice how the fees are directly written without settling fee accounting:
function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {
@> fees.push(Fee(_receiver, _feeBasisPoints));
require(_totalFeesBasisPoints() <= 4000, "Total fees must be <= 40%");
}
https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L347C5-L350C6
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;
}
require(_totalFeesBasisPoints() <= 4000, "Total fees must be <= 40%");
}
https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L358C5-L374C6
This changes the share of the rewards distribution for pending due rewards, resulting in undue gains or losses for fees accrued against the terms of the previous configuration.
Conversely, we note that the VaultControllerStrategy is careful to avoid this:
function addFee(address _receiver, uint256 _feeBasisPoints) external onlyOwner {
@> _updateStrategyRewards();
fees.push(Fee(_receiver, _feeBasisPoints));
if (_totalFeesBasisPoints() > 3000) revert FeesTooLarge();
}
https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/base/VaultControllerStrategy.sol#L649C5-L653C6
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();
}
https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/base/VaultControllerStrategy.sol#L663C5-L679C6
Impact
Leads to the overestimation or underestimation of rewards resulting in:
Undue claims on pending fees.
The loss of due fees for affected fee recipients (i.e. those either removed or assigned a lower fee bps).
Tools Used
Manual Review
Recommendations
Claim the rewards via _updateStrategyRewards before applying fee updates.
