Liquid Staking
Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Updating Strategy Rewards on StakingPool when totalFeeAmounts is equals or greather than the totalStaked causes the rewards to be lost.

0xstalin

Summary

When updating the strategy rewards on the StakingPool, there is a scenario when the totalFeeAmounts >= totalStaked which causes the rewards to be lost because no shares will minted.

Vulnerability Details

There is a scenario when updating strategy rewards on the StakingPool that causes the rewards to be lost because no shares will be minted to the feeReceivers.

StakingPool._updateStrategyRewards()

function _updateStrategyRewards(uint256[] memory _strategyIdxs, bytes memory _data) private {
...
​
// sum up rewards and fees across strategies
for (uint256 i = 0; i < _strategyIdxs.length; ++i) {
IStrategy strategy = IStrategy(strategies[_strategyIdxs[i]]);
​
//@audit => Update deposits on all strategies and computes the totalRewards to be distributed to feeReceivers.
//@audit => There is no comming back once deposits have been updated on the Strategy, totalDeposits ends up accounting for the newly earned rewards.
(
int256 depositChange,
address[] memory strategyReceivers,
uint256[] memory strategyFeeAmounts
) = strategy.updateDeposits(_data);
totalRewards += depositChange;
​
...
}
​
//@audit => There is no need to update `totalStaked` before the shares have been minted!
// 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;
​
//@audit => Computes the feeAmounts that each feeReceiver is entitled to receive.
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];
}
}
​
//@audit-issue => This safety check is what causes the fees to be lost.
//@audit-issue => Setting `totalFeeAmounts` to 0 causes the next block of code to be skipped, therefore, no shares are minted to the feeReceivers for their earned rewards.
// safety check
if (totalFeeAmounts >= totalStaked) {
totalFeeAmounts = 0;
}
​
//@audit => Because of the previous safety check, this block of code is skipped, therefore, no shares are minted and distributed to the fee receivers.
// distribute fees to receivers if there are any
if (totalFeeAmounts > 0) {
...
}
​
emit UpdateStrategyRewards(msg.sender, totalStaked, totalRewards, totalFeeAmounts);
}

An example when totalFeeAmounts >= totalStaked would be true is when there are no tokens staked because all the deposits were withdrawn, but, there are still pending rewards to be distributed, in this case, totalFeeAmount will be the same as totalStaked, which would cause the rewards being distributed to be lost, the feeReceivers won't receive any shares to claim those rewards.

Impact

Strategy rewards are lost because shares are not minted when totalFeeAmounts >= totalStaked.

Tools Used

Manual Audit

Recommendations

Refactor the logic to mint the corresponding shares for rewards.

  1. There is no need to update the totalStaked before minting the shares, use the same approach as when minting shares for deposits, first mint shares and then update totalStaked.

  2. Remove the safety check, when totalFeeAmounts >= totalStaked it means that the shares for the fees should be minted at a 1:1 ratio because there are no tokens staked on the pool, a.k.a, totalStaked == 0

  3. Update the current approach for minting the shares for the fees. Instead, call the _mint() and pass the totalFeeAmounts as a parameter, let the _mint() to compute the amount of shares that should be minted for the totalFees, the same approach as for deposits.

The changes on the code would look like these:

StakingPool._updateStrategyRewards()

function _updateStrategyRewards(uint256[] memory _strategyIdxs, bytes memory _data) private {
...
​
//@audit => Do not update `totalStaked` yet, first mint shares and then update `totalStaked`
- // update totalStaked if there was a net change in deposits
- if (totalRewards != 0) {
- totalStaked = uint256(int256(totalStaked) + totalRewards);
- }
​
...
​
//@audit => Remove this check, this is exactly what causes the rewards to be lost
- // safety check
- if (totalFeeAmounts >= totalStaked) {
- totalFeeAmounts = 0;
- }
​
// distribute fees to receivers if there are any
if (totalFeeAmounts > 0) {
//@audit => Update the logic to mint the shares, remove these lines and add the below ones.
- uint256 sharesToMint = (totalFeeAmounts * totalShares) /
- (totalStaked - totalFeeAmounts);
- _mintShares(address(this), sharesToMint);
​
//@audit => Use the same approach to mint shares as when minting shares for deposits.
//@audit => First mint shares and then update `totalStaked`
+ _mint(address(this), totalFeeAmounts);
+ totalStaked = uint256(int256(totalStaked) + totalRewards);
​
...
}
​
emit UpdateStrategyRewards(msg.sender, totalStaked, totalRewards, totalFeeAmounts);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Appeal created

0xstalin Submitter
7 months ago
inallhonesty Lead Judge
7 months ago
inallhonesty Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.