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

removeSplitter Function Fails When the Splitter Has Accrued Rewards

tendency

Summary

Calls by the owner to the LSTRewardsSplitterController::removeSplitter function always revert when the splitter has accrued rewards, due to the incorrect use of an outdated balance when calling the splitter's withdraw function.

Vulnerability Details

The LSTRewardsSplitterController::removeSplitter function is intended to be called by the contract owner to remove an account's splitter. During this action, the splitter balance is checked, and if it is non-zero and not equal to the principal deposits, the splitter::splitRewards function is called to distribute the rewards among the fee receivers.

The problem arises because, after this action, the splitter::withdraw function is called with the previously checked splitter token balance instead of an updated balance, which results in the call always reverting under these conditions.

  • To better illustrate:

Let's assume the splitter balance is 200 and the principal deposits are 100. During the split, assuming total fees are 50%, 50% of 100 equals 50, making the new principal deposits equal to 100 + 50, which results in 150. Thus, the new balance becomes 150.

However, the function then tries to withdraw using prior balance, causing a revert since 150 - 200 will result in an underflow revert:

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) {
//<--- @audit-issue
if (balance != principalDeposits) splitter.splitRewards();
splitter.withdraw(balance, _account);
}
// ...
}

I have added a runnable POC demonstrating the issue below. Please add the following test to test/core/lst-rewards-splitter.test.ts, and then run:

yarn test --grep "removing a splitter should revert if the splitter has accrued reward"
it('removing a splitter should revert if the splitter has accrued reward', async () => {
const { accounts, controller, token, splitter0 } = await loadFixture(deployFixture);
// Deposit into the splitter
await token.transferAndCall(controller.target, toEther(100), '0x');
​
// Transfer reward
await token.transfer(splitter0.target, toEther(100));
​
expect(await token.balanceOf(splitter0.target)).to.not.equal(await splitter0.principalDeposits());
// Should fail
await expect(controller.removeSplitter(accounts[0])).to.be.reverted;
});

Here are the logs:

yarn run v1.22.22
$ npx hardhat test --network hardhat --grep 'removing a splitter should revert if the splitter has accrued reward'
​
LSTRewardsSplitter
✔ removing a splitter should revert if the splitter has accrued reward (9745ms)
​
1 passing (10s)
​
Done in 19.06s.

Impact

  • A malicious user can force a revert by sending some lst tokens to the splitter, hence blocking the removal of the splitter.

  • Even without malicious intervention, the function will always revert whenever the splitter to be removed has accrued rewards.

Tools Used

Manual Review

Recommendations

Update LSTRewardsSplitterController::removeSplitter to:

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();
// Use the new balance
splitter.withdraw(IERC20(lst).balanceOf(address(splitter)), _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 11 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.