Summary
The StakingPool::_updateStrategyRewards function retrieves all the fee receivers of all the strategies on the staking pool, calculates their respective fee amounts and sends the calculated fee amounts to all the fee receivers accordingly.
However, there are cases where the last fee receiver in the list of receivers could receive more or less than the fee amount due to them.
Vulnerability Details
The StakingPool::_updateStrategyRewards function calculates the fee amounts for each fee receiver according to their various basis points, saving each fee amount in an array to correspond to each fee receiver address.
The issue lies with the manner of distribution of rewards for each fee receiver. All fee receivers are assigned the exact fee amount due to them as calculated except for the last fee receiver on the list who receives an arbitrary amount. Since the last fee receiver is assigned balanceOf(address(this)) instead of feeAmounts[i][j], that makes it arbitrary and can pose some accounting issues. See code snippet below:
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++) {
receivers[receivers.length - 1][i] = fees[i].receiver;
feeAmounts[feeAmounts.length - 1][i] =
(uint256(totalRewards) * fees[i].basisPoints) /
10000;
totalFeeAmounts += feeAmounts[feeAmounts.length - 1][i];
}
}
if (totalFeeAmounts >= totalStaked) {
totalFeeAmounts = 0;
}
if (totalFeeAmounts > 0) {
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++;
}
}
}
}
emit UpdateStrategyRewards(msg.sender, totalStaked, totalRewards, totalFeeAmounts);
}
Here is the github link to the above code snippet https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L522-L600
Impact
The StakingPool::_updateStrategyRewards function will work without any accounting issues if and only if the StakingPool only holds the rewards due to all fee receivers as its balance, that way, every fee receiver will receive exactly what is due to them.
If however, the StakingPool holds more than just the strategy rewards in its balance, the protocol will run into issues because the last fee receiver will receive the entire balance in the StakingPool which is higher than their reward. On the other hand, if the StakingPool holds less than the strategy rewards in its balance such that it can pay all receivers their fee amount in full except the last fee receiver, the protocol will pay the last fee receiver less than their reward.
From another perspective, using an arbitrary amount such as balanceOf(address(this)) makes it difficult for the protocol to play safe when there is an issue such as if the StakingPool holds less than the strategy rewards in its balance such that it can pay all receivers their fee amount in full except the last fee receiver. Instead of reverting the entire transactions since the last fee receiver cannot receive their full amount, the process will succeed and the last fee receiver will be underpaid. But using feeAmounts[i][j] instead of balanceOf(address(this)) will keep the protocol from running into issues since the entire process will revert if any fee receiver does not receiver their just reward.
Tools Used
Manual Review
Recommendations
Consider modifying the function to use feeAmounts[i][j] as the amount to credit all fee receivers and not use an arbitrary amount of balanceOf(address(this)) for the last fee receiver.
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[][]();
// sum up rewards and fees across strategies
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];
}
}
}
// update totalStaked if there was a net change in deposits
if (totalRewards != 0) {
totalStaked = uint256(int256(totalStaked) + totalRewards);
}
// calulate fees if net positive rewards were earned
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++) {
receivers[receivers.length - 1][i] = fees[i].receiver;
feeAmounts[feeAmounts.length - 1][i] =
(uint256(totalRewards) * fees[i].basisPoints) /
10000;
totalFeeAmounts += feeAmounts[feeAmounts.length - 1][i];
}
}
// safety check
if (totalFeeAmounts >= totalStaked) {
totalFeeAmounts = 0;
}
// distribute fees to receivers if there are any
if (totalFeeAmounts > 0) {
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++) {
+ transferAndCallFrom(address(this), receivers[i][j], feeAmounts[i][j], "0x");
- 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++;
- }
}
}
}
emit UpdateStrategyRewards(msg.sender, totalStaked, totalRewards, totalFeeAmounts);
}
