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

Improper Balance Handling in removeSplitter Function Leads to Fund Lock and Irremovable Splitters

cheatcode

Details

The removeSplitter function in the LSTRewardsSplitterController contract is designed to remove a splitter associated with an account, distribute any remaining rewards, and clean up the contract state.

The vulnerability lies in the function's failure to properly handle the case where the splitter's balance is less than its recorded principal deposits. This can lead to a situation where funds are locked in the splitter contract.

Code Snippet

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];
​
// ... (rest of the function)
}

Impact

  1. Funds Lock-up: If the splitter's balance is less than its principalDeposits, the withdraw call will revert, preventing the splitter from being removed and potentially locking funds.

  2. Inconsistent State: The contract could end up in a state where a splitter can't be removed, leading to accumulation of "dead" splitters.

  3. Denial of Service: It could prevent the removal of splitters that have experienced losses, hampering contract management.

Scenario

  1. A splitter is created with 100 tokens as principal deposits.

  2. Due to some error or external factor, 10 tokens are lost or stuck (balance becomes 90).

  3. The owner tries to remove the splitter.

  4. The removeSplitter function checks the balance (90) and principalDeposits (100).

  5. It attempts to withdraw 90 tokens, but this fails because it's less than the principalDeposits.

  6. The splitter cannot be removed, and the remaining 90 tokens are locked.

Fix

To address this issue, we need to handle cases where the balance might be less than principalDeposits, and ensure we can always remove a splitter.

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);
​
emit SplitterRemoved(_account, balance, principalDeposits);
}

This fix ensures that the splitter can always be removed, even in scenarios where there's been a loss of funds. It withdraws whatever balance is available, updates the contract state, and emits an event with relevant information for off-chain tracking.

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.