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

Owner unable to remove splitter when calling `LSTRewardsSplitterController::removeSplitter(...)`

Z0NaN

Summary

The Owner is unable to remove an account's splitter when calling LSTRewardsSplitterController::removeSplitter(...)

Vulnerability Details

The owner calls this function: LSTRewardsSplitterController::removeSplitter(...)

​
function removeSplitter(address _account) external onlyOwner {
ILSTRewardsSplitter splitter = splitters[_account];
if (address(splitter) == address(0)) revert SplitterNotFound();
​
uint256 balance = IERC20(lst).balanceOf(address(splitter));
uint256 principalDeposits = splitter.principalDeposits();
if (balance != 0) {
if (balance != principalDeposits) splitter.splitRewards();
splitter.withdraw(balance, _account); // @audit this will revert if balance > principalDeposits, should be splitter.principalDeposits() instead of balance
}
​
// ...
}

If we consider that balance > principalDeposits which is a very likely scenario, splitter.splitRewards(); will be called followed by splitter.withdraw(balance, _account);

function splitRewards() external {
int256 newRewards = int256(lst.balanceOf(address(this))) - int256(principalDeposits);
if (newRewards < 0) {
principalDeposits -= uint256(-1 * newRewards);
} else if (newRewards == 0) {
revert InsufficientRewards();
} else {
_splitRewards(uint256(newRewards));
}
}

since we considered the scenario where balance > principalDeposits , newRewards > 0 and `_splitRewards(uint256(newRewards));` is executed:

function _splitRewards(uint256 _rewardsAmount) private {
for (uint256 i = 0; i < fees.length; ++i) {
Fee memory fee = fees[i];
uint256 amount = (_rewardsAmount * fee.basisPoints) / 10000;
​
if (fee.receiver == address(lst)) {
IStakingPool(address(lst)).burn(amount);
} else {
lst.safeTransfer(fee.receiver, amount); // @audit lst.balanceOf(address(this)) is decreasing
}
}
​
principalDeposits = lst.balanceOf(address(this)); // @audit principalDeposits < balance (set in removeSplitter(...))
emit RewardsSplit(_rewardsAmount);
}

If we consider that fee.receiver != address(lst) then lst.safeTransfer(fee.receiver, amount); will be executed and principalDeposits = lst.balanceOf(address(this)) < balance from removeSplitter(...)).

Now that splitter.splitRewards() was executed successfully, splitter.withdraw(balance, _account);is executed and that's what happens:

function withdraw(uint256 _amount, address _receiver) external onlyController { // ok
principalDeposits -= _amount; // @audit this will revert since principalDeposits(the balance after splitting rewards) < _amount(the balance before splitting rewards)
lst.safeTransfer(_receiver, _amount);
emit Withdraw(_amount);
}

Impact

This a broken functionality that prevents the owner from exercising his right of removing an account's splitter. This will only occur in the case where:

  • balance > principalDeposits: very likely

  • fee.receiver != address(lst): less likely

Likelihood: Medium

Impact: Medium

Tools Used

Manual analysis

Recommendations

Make the following change:

function removeSplitter(address _account) external onlyOwner {
//...
if (balance != principalDeposits) splitter.splitRewards();
- splitter.withdraw(balance, _account);
+ splitter.withdraw(splitter.principalDeposits(), _account);
}
//...
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

In `removeSplitter` the `principalDeposits` should be used as an input for `withdraw` instead of balance after splitting the existing rewards.

Support

FAQs

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