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

Updating deposits would fail for some strategies

bauchibred

Summary

Updating deposits would fail for some strategies causing core functionalities accross scope to be unreachable.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/OperatorVCS.sol#L159-L223

function updateDeposits(
bytes calldata _data
)
external
override
onlyStakingPool
returns (int256 depositChange, address[] memory receivers, uint256[] memory amounts)
{
..snip
for (uint256 i = 0; i < vaultCount; ++i) {
(uint256 deposits, uint256 principal, uint256 rewards) = IOperatorVault(
address(vaults[i])
).updateDeposits(minRewards, receiver);
vaultDeposits += deposits;
newTotalPrincipalDeposits += principal;
operatorRewards += rewards;
}
​
uint256 balance = token.balanceOf(address(this));
depositChange = int256(vaultDeposits + balance) - int256(totalDeposits);
​
if (operatorRewards != 0) {
receivers = new address[](1 + (depositChange > 0 ? fees.length : 0));
amounts = new uint256[](receivers.length);
receivers[1] = address(this);
amounts[1] = operatorRewards;
unclaimedOperatorRewards += operatorRewards;
}
​
if (depositChange > 0) {
newTotalDeposits += uint256(depositChange);
​
if (receivers.length == 0) {
receivers = new address[](fees.length);
amounts = new uint256[](receivers.length);
​
for (uint256 i = 0; i < receivers.length; ++i) {
receivers[i] = fees[i].receiver;
amounts[i] = (uint256(depositChange) * fees[i].basisPoints) / 10000;
}
} else {
for (uint256 i = 1; i < receivers.length; ++i) {
receivers[i] = fees[i - 1].receiver;
amounts[i] = (uint256(depositChange) * fees[i - 1].basisPoints) / 10000;
}
}
} else if (depositChange < 0) {
newTotalDeposits -= uint256(depositChange * -1);
}
..snip
}

This function is used to update deposit accounting and calculate fees on newly earned rewards to all beneficiaries. The issue, however, is that there is an edge case where the function reverts without any deposits being updated.

Note that there is a valid instance where we would expect our deposit change to be negative, which is why this block exists:

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/OperatorVCS.sol#L212-L215

} else if (depositChange < 0) {
newTotalDeposits -= uint256(depositChange * -1);
}

In that case, the change is deducted from the new deposits. The issue, however, is that the protocol assumes the value of depositChange in uint is always going to be less than newTotalDeposits. However, this is not enforced, which then causes the attempt at updating the deposits to revert when uint256(depositChange) > newTotalDeposits.

Impact

Core functionality is broken, considering the functionality is used heavily across scope. In one case, we have it being used to attempt to distribute rewards/fees based on balance changes in strategies since the last update in the staking pool: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L529-L548

function _updateStrategyRewards(uint256[] memory _strategyIdxs, bytes memory _data) private {
..snip
// 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];
}
}
}
..snip
}

It is key to note that this functionality is used when attempting to remove strategies and also when the rebase controller or the strategy itself is trying to update the rewards: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L413-L417.

This report shows how in all these instances, the attempt could revert and essentially mean that a strategy cannot get removed/deprecated (which should always be available).

Even worse, the rebase controller can't function properly as:

  • Upkeeps can't be performed in the case of a negative rebase or pauses the priority pool if losses exceed the maximum since the attempt would revert here.

  • Attempts to update the rewards by the rebase bot would also always fail here.

  • Even if the owner decides to reopen the pool due to them maybe being paused on the basis of a significant slashing event and rebases the staking pool, it also would always revert here for the strategy.

NB: All the functionalities listed above in this section should be available, however, as shown, that isn't the case and as such core functionalities would be broken

Tools Used

Manual review

Recommendations

} else if (depositChange < 0) {
- newTotalDeposits -= uint256(depositChange * -1);
​
+ uint256 UdepositChange = uint256(depositChange * -1);
+ if (UdepositChange < newTotalDeposits) { newTotalDeposits -= UdepositChange; }
+ else { newTotalDeposits = 0; }
}

Here is the report with the typos fixed:

Updates

Lead Judging Commences

inallhonesty Lead Judge 8 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.