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

Low Findings : L01 - L04

0xrs

L01 - LSTRewardsSplitterController::removeSplitter - LSTRewardSplitter cannot be removed if there are undistributed rewards

Link: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/lstRewardsSplitter/LSTRewardsSplitterController.sol#L138

The function evaluates the current splitter balance:

uint256 balance = IERC20(lst).balanceOf(address(splitter));

If there is a balance and the balance is different from the principalDeposits, splitRewards is called on the splitter that should be removed.

if (balance != principalDeposits) splitter.splitRewards();

And, finally, the withdraw function is called on the splitter:

splitter.withdraw(balance, _account);

The problem here is, that the initially retrieved balance value (see above) is passed to the withdraw function. However, after calling "splitRewards", the balance of the splitter will change (it will be lower).

In the LSTRewardsSplitter::withdraw function, the following line will be executed with an _amount value that is bigger than the principalDeposits value

principalDeposits -= _amount;

And, this will cause a panic error (arithmetic operation overflowed outside of an unchecked block)

Proof of concept:

Add the following test to lst-rewards-splitter.test.ts:

it('removeSplitter should fail if there are undistributed rewards', async () => {
const { accounts, controller, token, splitter0 } = await loadFixture(deployFixture)
​
await token.transferAndCall(controller.target, toEther(100), '0x')
await token.transfer(splitter0.target, toEther(100)) //simulate rewards
​
await expect(controller.removeSplitter(accounts[0])).to.be.reverted
})

Recommendations

Before calling splitter.withdraw... in the removeSplitter function, update the balance:

...
balance = IERC20(lst).balanceOf(address(splitter));
splitter.withdraw(balance, _account);
...

L02 - WithdrawalPool ::updateWithdrawalBatchIdCutoff - the withdrawalBatchIdCutoff is not correctly set

Link: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/priorityPool/WithdrawalPool.sol#L393

The value of newWithdrawalBatchIdCutoff is set at the end of the second for-loop, but it should be set at the beginning of the loop, because, if the "break" statement is executed, the correct value of newWithdrawalBatchIdCutoff won't be set.

Proof of concept:

Add the following test to withdrawal-pool.test.ts:

it('withdrawalBatchIdCutoff is not correctly set in updateWithdrawalBatchIdCutoff', async () => {
const { signers, accounts, withdrawalPool } = await loadFixture(deployFixture)
​
//we queue 3 withdrawals for accounts[0] => this will add 3 new Withdrawals (withdrawalId 1 to 3) to the queuedWithdrawals array
await withdrawalPool.queueWithdrawal(accounts[0], toEther(100))
await withdrawalPool.queueWithdrawal(accounts[0], toEther(100))
await withdrawalPool.queueWithdrawal(accounts[0], toEther(100))
//we make a first deposit of 200 => this will add a new WithdrawalBatch (withdrawalBatch 1) to the withdrawalBatches array
//and this will service withdrawalId 1 & 2 => indexOfNextWithdrawal = 3
await withdrawalPool.deposit(toEther(200))
​
//we make a second deposit of 100 => this will add a new WithdrawalBatch (withdrawalBatch 2) to the withdrawalBatches array
//and this will service withdrawalId 3 => indexOfNextWithdrawal = 4
await withdrawalPool.deposit(toEther(100))
//perform a withdraw for accounts[0] => but, only for withdrawalId 1 & 2 => both are in withdrawalBatch 1
await withdrawalPool.connect(signers[0]).withdraw([ 1,2 ], [1,1])
​
//before calling updateWithdrawalBatchIdCutoff, the withdrawalBatchIdCutoff will be 0
console.log("withdrawalBatchIdCutoff 1: ", await withdrawalPool.withdrawalBatchIdCutoff()) //0
assert.equal(await withdrawalPool.withdrawalBatchIdCutoff(), 0)
​
await withdrawalPool.updateWithdrawalBatchIdCutoff()
​
//after calling updateWithdrawalBatchIdCutoff, the withdrawalBatchIdCutoff should be 2, but it is actually 1
//all batches before withdrawalBatch 2 have had all withdrawal requests fully withdrawn => so, the correct value is 2 !
console.log("withdrawalBatchIdCutoff 2: ", await withdrawalPool.withdrawalBatchIdCutoff()) //1 => should be 2 !!!
//the following test fails until necessary corrections are made in updateWithdrawalBatchIdCutoff
assert.equal(await withdrawalPool.withdrawalBatchIdCutoff(), 2)
})

Recommendations

Replace the second for-loop in the updateWithdrawalBatchIdCutoff function with the following code:

...
for (uint256 i = newWithdrawalBatchIdCutoff; i < numBatches; ++i) {
newWithdrawalBatchIdCutoff = i;
if (withdrawalBatches[i].indexOfLastWithdrawal >= newWithdrawalIdCutoff) {
break;
}
​
newWithdrawalBatchIdCutoff = i;
}
...

L03 - StakingPool - Owner cannot change the maximum allowed value for totalFeesBasisPoints

Links:

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L76
https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L349
https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/core/StakingPool.sol#L373

The contract uses hardcoded ("magic number") values for the max allowed totalFeesBasisPoints, which prevents the contract owner from modifying this value.

Recommendations

Add an external setter function with an onlyOwner modifer and a corresponding state variable to the contract - similar to the setUnusedDepositLimit function.

L04 - OperatorVCS::initialize - when the contract is upgraded to version3 there is no need to add additional Vault Groups

Links: https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/OperatorVCS.sol#L85C9-L87C10

Vault Groups only need to be added when the contract is first deployed, when the contract is upgraded to version 3 (reinitializer(3)), those vaultGroups exist already and 5 additional Vault Groups will be added.

Recommendations

Place the code section that adds the Vault Groups within the if-block in the initialize function:

function initialize(
address _token,
address _stakingPool,
address _stakeController,
address _vaultImplementation,
Fee[] memory _fees,
uint256 _maxDepositSizeBP,
uint256 _vaultMaxDeposits,
uint256 _operatorRewardPercentage,
address _vaultDepositController
) public reinitializer(3) {
if (address(token) == address(0)) {
__VaultControllerStrategy_init(
_token,
_stakingPool,
_stakeController,
_vaultImplementation,
_fees,
_maxDepositSizeBP,
_vaultMaxDeposits,
_vaultDepositController
);
​
if (_operatorRewardPercentage > 10000) revert InvalidPercentage();
operatorRewardPercentage = _operatorRewardPercentage;
globalVaultState = GlobalVaultState(5, 0, 0, 0);
+ for (uint64 i = 0; i < 5; ++i) {
+ vaultGroups.push(VaultGroup(i, 0));
+ }
} else {
globalVaultState = GlobalVaultState(5, 0, 0, uint64(maxDepositSizeBP + 1));
maxDepositSizeBP = _maxDepositSizeBP;
delete fundFlowController;
vaultMaxDeposits = _vaultMaxDeposits;
}
​
- for (uint64 i = 0; i < 5; ++i) {
- vaultGroups.push(VaultGroup(i, 0));
- }
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
11 months ago
inallhonesty Lead Judge
11 months ago
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.

newWithdrawalBatchIdCutoff

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.