Summary
The `Remove Splitter` function will always revert when the reward in the splitter is more than 0. When the admin calls `Remove Splitter` in a scenario where the `Newreward` in the contract is greater than 0, the action reverts due to an issue in the logic handling reward splitting and withdrawal.
Vulnerability Details
The vulnerability lies in the 'removesplitter()` and subsequent withdrawal process. Specifically:
function removeSplitter(address _account) external onlyOwner {
ILSTRewardsSplitter splitter = splitters[_account];
if (address(splitter) == address(0)) revert SplitterNotFound();
@audit>> 1. >> uint256 balance = IERC20(lst).balanceOf(address(splitter));
@audit>> 2. >> uint256 principalDeposits = splitter.principalDeposits();
if (balance != 0) {
@audit>> 3. >> if (balance != principalDeposits) splitter.splitRewards();
@audit>> 4. Revert if balnace has reduced >> splitter.withdraw(balance, _account);
}
If the `balance` and `principalDeposits` differ, the contract splits the rewards and calls `withdraw`. However, after splitting the rewards, the balance is adjusted internally, but the `withdraw` function still attempts to use the old balance, leading to a revert.
Here’s the relevant section from the `splitRewards()` function:
* @notice Splits new rewards between fee receivers
* @dev bypasses rewardThreshold
*/
function splitRewards() external {
@audit>> 1. >> int256 newRewards = int256(lst.balanceOf(address(this))) - int256(principalDeposits);
if (newRewards < 0) {
principalDeposits -= uint256(-1 * newRewards);
} else if (newRewards == 0) {
revert InsufficientRewards();
} else {
@audit>> 2. reward > 0 >> _splitRewards(uint256(newRewards));
}
}
The internal `_splitRewards()` function either burns the fees or transfers tokens to the receiver. After the split, `principalDeposits` is adjusted.
* @notice Splits new rewards
* @param _rewardsAmount amount of new rewards
*/
function _splitRewards(uint256 _rewardsAmount) private {
for (uint256 i = 0; i < fees.length; ++i) {
Fee memory fee = fees[i];
@audit>> 1. get amount >> uint256 amount = (_rewardsAmount * fee.basisPoints) / 10000;
if (fee.receiver == address(lst)) {
@audit>> 2. burn >> IStakingPool(address(lst)).burn(amount);
} else {
@audit>> 3. send >> lst.safeTransfer(fee.receiver, amount);
}
}
@audit>> 4. reset principal to new balance not old >> principalDeposits = lst.balanceOf(address(this));
emit RewardsSplit(_rewardsAmount);
}
However, when the withdrawal is called afterward, it uses the old `balance`, which has been reduced, causing the transaction to revert.
function removeSplitter(address _account) external onlyOwner {
uint256 balance = IERC20(lst).balanceOf(address(splitter));
uint256 principalDeposits = splitter.principalDeposits();
if (balance != 0) {
if (balance != principalDeposits) splitter.splitRewards();
@audit>> 1. sending old balance after burn and transfer>> splitter.withdraw(balance, _account);
}
The problem arises because the `withdraw` function calls with the old `balance`, while the actual balance has already been reduced by the split operation.
* @notice Withdraws tokens
* @param _amount amount to withdraw
* @param _receiver address to receive tokens
*/
function withdraw(uint256 _amount, address _receiver) external onlyController {
@audit>> 1. revert>> principalDeposits -= _amount;
@audit>> 2. revert>> lst.safeTransfer(_receiver, _amount);
emit Withdraw(_amount);
}
Impact
This issue prevents the `Remove Splitter` function from completing when there are rewards in the contract.
Tools Used
Manual code Review
Recommendations
1. **Adjust Balance Handling**: After calling `splitRewards()`, ensure that the withdrawal function uses the updated balance (`address(this).balance`) instead of the original balance of the splitter.
function removeSplitter(address _account) external onlyOwner {
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);
}
