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

Incorrect Balance Handling in LSTRewardsSplitterController::removeSplitter Causes Revert on Splitter Removal

4rdiii

[M-01] Incorrect Balance Handling in LSTRewardsSplitterController::removeSplitter Causes Revert on Splitter Removal

Summary

The LSTRewardsSplitterController::removeSplitter function is designed to handle various conditions that a splitter might have. However, when the balance is not equal to 0 and not equal to principalDeposits, an issue arises. The balance variable is not updated after the LSTRewardsSplitter::splitRewards function is called, which can result in an arithmetic overflow and a revert. This prevents the owner from removing the splitter, even if desired.

Vulnerability Details

In the removeSplitter function, after the checks for balance != 0 and balance != principalDeposits, there is a call to the removing splitter to split any remaining rewards. This changes the splitter's balance, but the balance is not recalculated before the withdrawal operation, leading to a revert due to the mismatch between the old balance and the current balance.

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);
}
​
delete splitters[_account];
​
uint256 numAccounts = accounts.length;
for (uint256 i = 0; i < numAccounts; ++i) {
if (accounts[i] == _account) {
accounts[i] = accounts[numAccounts - 1];
accounts.pop();
break;
}
}
​
IERC677(lst).safeApprove(address(splitter), 0);
}

As shown above, the LSTRewardsSplitter::_splitReward function transfers some of the balance to fee receivers, which causes the balance of the splitter to change.

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);
}
}
​
principalDeposits = lst.balanceOf(address(this));
emit RewardsSplit(_rewardsAmount);
}

Impact

This issue can lead to situations where the owner of the controller is unable to remove a splitter, either intentionally or unintentionally, due to the outdated balance calculation.

Proof of Concept (POC)

The following test can be added to lst-rewards-splitter.test.ts to demonstrate the problem. This test sets up conditions that trigger a call to splitRewards, and checks for a reversion when trying to remove the splitter.

it('should be able to remove splitter', async () => {
const { signers, accounts, controller, token, splitter0, splitter1 } = await loadFixture(deployFixture)
​
await token.transferAndCall(controller.target, toEther(100), '0x')
await token.connect(signers[1]).transferAndCall(controller.target, toEther(200), '0x')
await token.transfer(splitter1.target, toEther(80))
​
// Checking that splitter1 balance and principalDeposits are indeed not equal!
console.log(await token.balanceOf(splitter1.target)) // 280 000 000 000 000 000 000
console.log(await splitter1.principalDeposits()) // 200 000 000 000 000 000 000
​
// Try to remove splitter1
await expect(controller.removeSplitter(accounts[1])).to.be.revertedWithPanic()
})

Tools Used

Manual Review

Recommendations

The fix is straightforward—update the balance after calling splitRewards to ensure the correct balance is used in the withdrawal.

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();
+ balance = IERC20(lst).balanceOf(address(splitter));
}
splitter.withdraw(balance, _account);
}
​
delete splitters[_account];
​
uint256 numAccounts = accounts.length;
for (uint256 i = 0; i < numAccounts; ++i) {
if (accounts[i] == _account) {
accounts[i] = accounts[numAccounts - 1];
accounts.pop();
break;
}
}
​
IERC677(lst).safeApprove(address(splitter), 0);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year 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.