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

Operator can't withdraw the tokens during `removeOperators`

0xtheblackpanther

Github

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/OperatorStakingPool.sol#L163-L184

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/OperatorStakingPool.sol#L199-L205

Summary

The removeOperators function in the OperatorStakingPool contract fails to actually transfer staked tokens back to operators when they are removed, despite updating internal accounting. This can lead to tokens being trapped in the contract and a mismatch between recorded and actual balances.

Vulnerability details

When an operator is removed via the removeOperators function, it calls the _withdraw function to handle any staked tokens:

function removeOperators(address[] calldata _operators) external onlyOwner {
// ... (other code)
uint256 staked = getOperatorStaked(operator);
if (staked != 0) {
_withdraw(operator, staked);
}
// ... (other code)
}

However, the _withdraw function only updates internal accounting without transferring tokens:

function _withdraw(address _operator, uint256 _amount) private {
uint256 sharesAmount = lst.getSharesByStake(_amount);
shareBalances[_operator] -= sharesAmount;
totalShares -= sharesAmount;
​
emit Withdraw(_operator, _amount, sharesAmount);
}

This function updates share balances and emits a Withdraw event, but doesn't actually transfer any tokens to the operator.

Impact

  1. Trapped Funds: Staked tokens remain in the contract after an operator is removed, potentially becoming inaccessible.

  2. Accounting Discrepancy: The contract's internal accounting no longer matches its actual token balance.

  3. Misleading Events: Withdraw events are emitted without corresponding token transfers.

  4. Trust Issues: Operators may lose trust in the system if they can't retrieve their staked tokens upon removal.

The severity is high due to the potential for permanent loss of user funds and the fundamental misalignment between the contract's stated behavior and its actual operation.

What causes the issue and where?

The issue is caused by an incomplete implementation of the _withdraw function in the OperatorStakingPool contract. While it updates internal accounting, it fails to include the crucial step of transferring tokens back to the operator.

Proof of concept

  1. Admin adds an operator via addOperators.

  2. Operator stakes 100 LST tokens.

  3. Admin removes the operator via removeOperators.

  4. The operator's internal balance is set to 0, and a Withdraw event is emitted.

  5. However, the 100 LST tokens remain in the OperatorStakingPool contract.

  6. The operator has no way to retrieve their staked tokens.

Recommendations

Make sure there is a code which transfer the operator funds, here is two suggestions I have:

  • Modify the _withdraw function to include an actual token transfer:

function _withdraw(address _operator, uint256 _amount) private {
uint256 sharesAmount = lst.getSharesByStake(_amount);
shareBalances[_operator] -= sharesAmount;
totalShares -= sharesAmount;
​
lst.safeTransfer(_operator, _amount); // Add this line
​
emit Withdraw(_operator, _amount, sharesAmount);
}

  • Add a separate function for operators to claim any remaining balance after removal, in case the automatic withdrawal fails:

function claimRemainingBalance() external {
uint256 balance = getOperatorStaked(msg.sender);
require(balance > 0, "No balance to claim");
require(!isOperator(msg.sender), "Active operators cannot claim");
_withdraw(msg.sender, balance);
}

By implementing these recommendations, the contract will ensure that operators can always retrieve their staked tokens, maintaining the integrity and trustworthiness of the staking system.

Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

`OperatorStakingPool::_withdraw()` function doesn't transfer the tokens

Support

FAQs

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