Summary
Stakelink users can register to share their LST rewards with other addresses through the LSTRewardsSplitterController contract. Upon registration, the user will set fee addresses, and an individual LSTRewardSplitter contract will be created for the user.
function addSplitter(
address _account,
LSTRewardsSplitter.Fee[] memory _fees
) external onlyOwner {
if (address(splitters[_account]) != address(0)) revert SplitterAlreadyExists();
address splitter = address(new LSTRewardsSplitter(lst, _fees, owner()));
splitters[_account] = ILSTRewardsSplitter(splitter);
...
All user-accrued rewards are distributed to fee addresses in the individual LSTRewardsSplitter contract inside the _splitRewards function.
* @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];
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);
}
When a user wants to remove a splitter, the removeSplitter function will be called on the LSTRewardsSplitterController. Upon removing the splitter, all rewards at that moment should be distributed to the fee addresses. However, if there are some rewards left, this will always fail because the controller contract will try to transfer the entire balance of the splitter contract to the user, including previously distributed rewards. This will revert because the balance is not reduced by the already distributed rewards.
* @notice Removes an account's splitter
* @param _account address of account
**/
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);
}
Vulnerability Details
Vulnerable code: https://github.com/Cyfrin/2024-09-stakelink/blob/main/contracts/core/lstRewardsSplitter/LSTRewardsSplitterController.sol#L138
PoC
it('PoC:Medium:LSTRewardsSplitterController#134-138=>remove splitter will always revert if there are some rewards left on splitter contract', async () => {
const { accounts, controller, token } = await loadFixture(deployFixture)
await controller.addSplitter(accounts[2], [
{ receiver: accounts[7], basisPoints: 4000 },
{ receiver: accounts[8], basisPoints: 4000 },
])
const splitter = await ethers.getContractAt(
'LSTRewardsSplitter',
await controller.splitters(accounts[2])
)
const rewardAmount = toEther(100)
await token.transfer(splitter.target, rewardAmount)
await expect(controller.removeSplitter(accounts[2])).to.be.reverted;
})
Running Poc:
Copy test to ./test/core/lst-rewards-splitter.test.ts
Run tests with npx hardhat test ./test/core/lst-rewards-splitter.test.ts --network hardhat
Output
LSTRewardsSplitter
✔ PoC:Medium:LSTRewardsSplitterController#134-138=>remove splitter does not work when there are some rewards left on splitter contract (1252ms)
Impact
Likelihood: Medium
This will happen on every call to removeSplitter function inside LSTRewardsSplitterController if some rewards are accured.
Impact: Medium
The functionality of removing the splitter will be broken. The only way to remove the splitter would be to require that accrued rewards are zero. To ensure always correct functionality, the rewards would need to be distributed within the same block.
Tools Used
Manual review, hardhat tests.
Recommendations
After splitRewards call, only lst balance left on splitter contract should be transfered to user. This amount is captured at the end of _splitRewardsFunction inside principalDeposits state variable.
/**
* @notice Removes an account's splitter
* @param _account address of account
**/
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(splitter.principalDeposits(), _account);
}
A new test can be added to show that the functionality is now satisfied.
it('remove splitter should work when some rewards are left on splitter contract', async () => {
const { accounts, controller, token } = await loadFixture(deployFixture)
await controller.addSplitter(accounts[2], [
{ receiver: accounts[7], basisPoints: 4000 },
{ receiver: accounts[8], basisPoints: 4000 },
])
const splitter = await ethers.getContractAt(
'LSTRewardsSplitter',
await controller.splitters(accounts[2])
)
const rewardAmount = toEther(100)
await token.transfer(splitter.target, rewardAmount)
const firstReceiverBalanceBefore = await token.balanceOf(accounts[7])
const secondReceiverBalanceBefore = await token.balanceOf(accounts[8])
const splitterBalanceBefore = await token.balanceOf(splitter.target)
const splitterPrincipalDepositsBefore = await splitter.principalDeposits()
const accountBalanceBefore = await token.balanceOf(accounts[2])
console.log('\n=================BEFORE REMOVE SPLITTER====================')
console.log('firstReceiverBalanceBefore', firstReceiverBalanceBefore.toString())
console.log('secondReceiverBalanceBefore', secondReceiverBalanceBefore.toString())
console.log('splitterBalanceBefore', splitterBalanceBefore.toString())
console.log('splitterPrincipalDepositsBefore', splitterPrincipalDepositsBefore.toString())
console.log('accountBalanceBefore', accountBalanceBefore.toString())
assert.equal(splitterBalanceBefore - splitterPrincipalDepositsBefore, BigInt(rewardAmount))
await controller.removeSplitter(accounts[2])
const firstReceiverBalanceAfter = await token.balanceOf(accounts[7])
const secondReceiverBalanceAfter = await token.balanceOf(accounts[8])
const splitterBalanceAfter = await token.balanceOf(splitter.target)
const accountBalanceAfter = await token.balanceOf(accounts[2])
console.log('\n=================AFTER REMOVE SPLITTER====================')
console.log('firstReceiverBalanceAfter', firstReceiverBalanceAfter.toString())
console.log('secondReceiverBalanceAfter', secondReceiverBalanceAfter.toString())
console.log('splitterBalanceAfter', splitterBalanceAfter.toString())
console.log('accountBalanceAfter', accountBalanceAfter.toString())
assert.equal(firstReceiverBalanceAfter - firstReceiverBalanceBefore, BigInt(rewardAmount) * 4n / 10n)
assert.equal(secondReceiverBalanceAfter - secondReceiverBalanceBefore, BigInt(rewardAmount) * 4n / 10n)
assert.equal(splitterBalanceAfter, 0n)
assert.equal(accountBalanceAfter, accountBalanceBefore + BigInt(rewardAmount) * 2n / 10n)
})
Output:
LSTRewardsSplitter
=================BEFORE REMOVE SPLITTER====================
firstReceiverBalanceBefore 0
secondReceiverBalanceBefore 0
splitterBalanceBefore 100000000000000000000
splitterPrincipalDepositsBefore 0
accountBalanceBefore 10000000000000000000000
=================AFTER REMOVE SPLITTER====================
firstReceiverBalanceAfter 40000000000000000000
secondReceiverBalanceAfter 40000000000000000000
splitterBalanceAfter 0
accountBalanceAfter 10020000000000000000000
✔ remove splitter should work when some rewards are left on splitter contract (1286ms)
1 passing (1s)
