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

Rewards are paied even though the vault is net negative when accounting for lost deposits

Z0NaN

Summary

This issue resides in the OperatorVault contract as it distributes rewards to OperatorVCSwhen it shouldn't.

Vulnerability Details

These are possible flows that lead to this issue:

  • StakingPool::removeStrategy(...)->StakingPool::_updateStrategyRewards(...)->

    OperatorVCS::updateDeposit(...)->OperatorVault::updateDeposit(...)

  • StakingPool::updateStrategyRewards(...)->StakingPool::_updateStrategyRewards(...)->

    OperatorVCS::updateDeposit(...)->OperatorVault::updateDeposit(...)

/**
* @notice Updates the deposit and reward accounting for this vault
* @dev will only pay out rewards if the vault is net positive when accounting for lost deposits
* ...
*/
function updateDeposits(
uint256 _minRewards,
address _rewardsReceiver
) external onlyVaultController returns (uint256, uint256, uint256) {
uint256 principal = getPrincipalDeposits();
uint256 rewards = getRewards();
uint256 totalDeposits = principal + rewards;
int256 depositChange = int256(totalDeposits) - int256(uint256(trackedTotalDeposits));
​
uint256 opRewards;
if (depositChange > 0) {
opRewards =
(uint256(depositChange) *
IOperatorVCS(vaultController).operatorRewardPercentage()) /
10000;
unclaimedRewards += SafeCast.toUint128(opRewards);
trackedTotalDeposits = SafeCast.toUint128(totalDeposits);
}
​
if (_minRewards != 0 && rewards >= _minRewards) {
rewardsController.claimReward();
trackedTotalDeposits -= SafeCast.toUint128(rewards);
totalDeposits -= rewards;
token.safeTransfer(_rewardsReceiver, rewards);
} // @audit if depositChange < 0 rewards will get paied through `safeTransfer(...)` even though the natspec stated otherwise
​
return (totalDeposits, principal, opRewards);
}

Unlike the what's specified natspec @dev note, rewards will be distributed to the `rewardsReceiver` even though the vault is not net positive.

Impact

The rewards are being sent to OperatorVCS when they shouldn't be, this will result in a bad balance in `OperatorVCS`, hence a bad depositChange

uint256 balance = token.balanceOf(address(this));
depositChange = int256(vaultDeposits + balance) - int256(totalDeposits);

depositChange is a value returned by the OperatorVCS::updateDeposits() function, that will result in a bad totalRewards in StakingPool when updating the strategies:

totalRewards += depositChange;

This will result in a bad `totalStaked`:

if (totalRewards != 0) { // ok
totalStaked = uint256(int256(totalStaked) + totalRewards); // @audit bad casting if totalRewards < 0 and abs(totalRewards) > totalStaked
}

a bad `totalFeeAmounts` and hence a bad `sharesToMint`:

for (uint256 i = 0; i < fees.length; i++) { // ok
receivers[receivers.length - 1][i] = fees[i].receiver; // ok
feeAmounts[feeAmounts.length - 1][i] = // ok
(uint256(totalRewards) * fees[i].basisPoints) / // ok
10000; // ok @audit 10000 or totalFeesBasisPoints()??
totalFeeAmounts += feeAmounts[feeAmounts.length - 1][i]; // ok
}
if (totalFeeAmounts > 0) { // ok
uint256 sharesToMint = (totalFeeAmounts * totalShares) / // ok
(totalStaked - totalFeeAmounts); // ok
_mintShares(address(this), sharesToMint); // ok

Tools Used

Manual analysis

Recommendations

Make the following change:

/**
* @notice Updates the deposit and reward accounting for this vault
* @dev will only pay out rewards if the vault is net positive when accounting for lost deposits
* ...
*/
function updateDeposits(
uint256 _minRewards,
address _rewardsReceiver
) external onlyVaultController returns (uint256, uint256, uint256) {
uint256 principal = getPrincipalDeposits();
uint256 rewards = getRewards();
uint256 totalDeposits = principal + rewards;
int256 depositChange = int256(totalDeposits) - int256(uint256(trackedTotalDeposits));
​
uint256 opRewards;
if (depositChange > 0) {
opRewards =
(uint256(depositChange) *
IOperatorVCS(vaultController).operatorRewardPercentage()) /
10000;
unclaimedRewards += SafeCast.toUint128(opRewards);
trackedTotalDeposits = SafeCast.toUint128(totalDeposits);
+ if (_minRewards != 0 && rewards >= _minRewards) {
+ rewardsController.claimReward();
+ trackedTotalDeposits -= SafeCast.toUint128(rewards);
+ totalDeposits -= rewards;
+ token.safeTransfer(_rewardsReceiver, rewards);
+ }
}
- if (_minRewards != 0 && rewards >= _minRewards) {
- rewardsController.claimReward();
- trackedTotalDeposits -= SafeCast.toUint128(rewards);
- totalDeposits -= rewards;
- token.safeTransfer(_rewardsReceiver, rewards);
- }
return (totalDeposits, principal, opRewards);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
8 months ago
inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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