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

The `LSTRewardsSplitterController::removeSplitter` reverts due to arithmetic overflow or underflow whenever the splitter has positive balance

spomaria

Summary

The LSTRewardsSplitterController::removeSplitter is designed to remove a splitter contract from the array of splitters. The function executes by splitting any available rewards and withdraws any balances from the splitter contract before removing the splitter from the array of splitters.
However, this function will always revert for any splitter contract that has some funds in it, breaking the functionality of the protocol.

Vulnerability Details

The LSTRewardsSplitterController::removeSplitter function reverts for any splitter contract with a positive balance because during function execution, the balance of the splitter contract is cached as balance, the principal deposits in the splitter contract is cached as principalDeposits.

The function then checks if balance is zero or not. Where the balance is zero, the function works fine.

The problem lies where the balance is greater than zero. This is because, the function checks if balance != principalDeposits. Where balance is not equal to principalDeposits, it means there are rewards for distribution and the splitter will distribute the rewards. Now, after distributing rewards, the protocol attempts to withdraw the cached balance from the splitter contract whereas, the tokens remaining in the splitter contract is now less than the cached balance. Since the protocol attempts to withdraw more tokens from the splittr contract than the current balance, it triggers an arithmetic overflow or underflow causing the call to revert.

The code snippet is shown below and can be further confirmed using the following github link https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/lstRewardsSplitter/LSTRewardsSplitterController.sol#L130-L153

function removeSplitter(address _account) external onlyOwner {
ILSTRewardsSplitter splitter = splitters[_account];
if (address(splitter) == address(0)) revert SplitterNotFound();
​
​
@> uint256 balance = IERC20(lst).balanceOf(address(splitter)); // @audit-comment note that splitter balance is cached before spliting rewards
uint256 principalDeposits = splitter.principalDeposits();
if (balance != 0) {
if (balance != principalDeposits) splitter.splitRewards();
@> splitter.withdraw(balance, _account); // @audit-comment note that protocol attempts to withdraw `balance` from splitter when splitter balance must have reduced after spliting rewards
}
​
​
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);
}

Impact

Since the LSTRewardsSplitterController::removeSplitter reverts whenever the splitter contract has some tokens in it, the protocol fails to remove any splitter contract that has positive balance even if it is desirable to do so. This affects the protocol's functionality.

Tools Used

Manual Review and Hardhat

Proof of Concept:

  1. Add a splitter to the protocol

  2. Send some tokens to the splitter so that the splitter has a positive balance

  3. Attempt to remove the splitter from the array of splitters on the protocol. The execution will fail due to arithmetic underflow or overflow causing the call to revert

PoC Place the following code in `lst-rewards-splitter.test.ts`.
it.only('should fail to remove splitter with positive balance', async () => {
const { accounts, controller, token, splitter1 } = await loadFixture(deployFixture)
​
// send some tokens to splitter1
await token.transfer(splitter1.target, toEther(200))
​
// attempt to remove splitter1 from array of splitters. Call reverts
// reverts due to arithmetic overflow or underflow
await expect(controller.removeSplitter(accounts[1])).to.be.revertedWithPanic(0X11)
​
// const err = await controller.removeSplitter(accounts[1])
// console.log(err)
​
})

Run: yarn test

Output:

.
.
.
Compiled 8 Solidity files successfully (evm target: london).
​
​
LSTRewardsSplitter
✔ should fail to remove splitter with positive balance (10371ms)
​
​
1 passing (10s)
​

The test reverts as expected. To confirm that the test reverts due to arithmetic overflow or underflow, we can slightly adjust the code snippet for the test above to see the error message. Consider modifying the code snippet as below:

it.only('should fail to remove splitter with positive balance', async () => {
const { accounts, controller, token, splitter1 } = await loadFixture(deployFixture)
​
// send some tokens to splitter1
await token.transfer(splitter1.target, toEther(200))
​
// reverts due to arithmetic overflow or underflow
// await expect(controller.removeSplitter(accounts[1])).to.be.revertedWithPanic(0X11)
​
// attempt to remove splitter1 from array of splitters. Save the message as `err`
const err = await controller.removeSplitter(accounts[1])
console.log(err)
​
})

Run: yarn test

Output:

.
.
.
​
LSTRewardsSplitter
1) should fail to remove splitter with positive lst balance
​
​
0 passing (4s)
1 failing
​
1) LSTRewardsSplitter
should fail to remove splitter with positive balance:
Error: VM Exception while processing transaction: reverted with panic code 0x11 (Arithmetic operation overflowed outside of an unchecked block)
at LSTRewardsSplitter.withdraw (contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol:81)
at LSTRewardsSplitterController.removeSplitter (contracts/core/lstRewardsSplitter/LSTRewardsSplitterController.sol:143)
at EdrProviderWrapper.request (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:446:41)
at async HardhatEthersSigner.sendTransaction (node_modules/@nomicfoundation/hardhat-ethers/src/signers.ts:125:18)
at async send (node_modules/ethers/src.ts/contract/contract.ts:313:20)
at async Proxy.removeSplitter (node_modules/ethers/src.ts/contract/contract.ts:352:16)
at async Context.<anonymous> (test/core/lst-rewards-splitter.test.ts:284:17)
​

The above output confirms that the call reverts due to arithmetic overflow or underflow.

Recommendations

Consider modifying the LSTRewardsSplitterController::removeSplitter function such that it withdraws the current balance of the splitter contract after splitting rewards rather than attempting to withdraw an amount that corresponds to the splitter balance cached before splitting rewards.

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);
+ 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);
}

With this adjustment, the LSTRewardsSplitterController::removeSplitter function will execute as expected, enhancing the protocol's functionality.

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.