Summary
Inside OperatorStakingPool, node operators are required to stake their LSTs. Total LST staked and node operators balances are tracked by state variables:
mapping(address => uint256) private shareBalances;
uint256 private totalShares;
Upon withdrawing (OperatorStakingPool:_withdraw:L200-204) sharesBalances and totalShares are updated but no LST is transfered from OperatorStakingPool back to operator. This leaves OperatorStakingPool with stuck LST tokens and node operators can't withdraw their stake.
function withdraw(uint256 _amount) external {
if (!isOperator(msg.sender)) revert SenderNotAuthorized();
_withdraw(msg.sender, _amount);
}
function _withdraw(address _operator, uint256 _amount) private {
uint256 sharesAmount = lst.getSharesByStake(_amount);
@> shareBalances[_operator] -= sharesAmount;
@> totalShares -= sharesAmount;
emit Withdraw(_operator, _amount, sharesAmount);
}
Vulnerability Details
Vulnerable code: https://github.com/Cyfrin/2024-09-stakelink/blob/main/contracts/linkStaking/OperatorStakingPool.sol#L199
PoC
it('PoC:High:OperatorStakingPool.sol#L199-204=>No LSTs transfer on node operator withdrawals resulting in stuck funds', async () => {
const { signers, accounts, opPool, lst } = await loadFixture(deployFixture)
const operator = signers[0]
const operatorDepositAmount = toEther(1000)
const operatorBalanceBeforeDeposit = await lst.balanceOf(operator.address)
const opStakingPoolBalanceBeforeDeposit = await lst.balanceOf(opPool.target)
await lst.connect(operator).transferAndCall(opPool.target, operatorDepositAmount, '0x')
const operatorBalanceAfterDeposit = await lst.balanceOf(operator.address)
const opStakingPoolBalanceAfterDeposit = await lst.balanceOf(opPool.target)
assert.equal(operatorBalanceBeforeDeposit - operatorBalanceAfterDeposit, operatorDepositAmount)
assert.equal(opStakingPoolBalanceAfterDeposit, opStakingPoolBalanceBeforeDeposit + operatorDepositAmount)
const operatorBalanceBeforeWithdraw = await lst.balanceOf(operator.address)
const opStakingPoolBalanceBeforeWithdraw = await lst.balanceOf(opPool.target)
await opPool.connect(operator).withdraw(operatorDepositAmount)
const operatorBalanceAfterWithdraw = await lst.balanceOf(operator.address)
const opStakingPoolBalanceAfterWithdraw = await lst.balanceOf(opPool.target)
assert.equal(fromEther(await opPool.getOperatorPrincipal(accounts[0])), 0)
assert.equal(fromEther(await opPool.getOperatorStaked(accounts[0])), 0)
assert.equal(operatorBalanceAfterWithdraw, operatorBalanceBeforeWithdraw)
assert.equal(opStakingPoolBalanceAfterWithdraw, opStakingPoolBalanceBeforeWithdraw)
})
Running Poc:
Copy test to ./test/linkStaking/operator-staking-pool.test.ts
Run tests with npx hardhat test ./test/linkStaking/operator-staking-pool.test.ts --network hardhat
Output:
OperatorStakingPool
✔ PoC:High:OperatorStakingPool.sol#L200-204=>No LSTs transfer on operator withdrawals resulting in stuck funds (1499ms)
Impact
Likelihood: High
This will happen on every call to withdraw function inside OperatorStakingPool.
Also when owner calls removeOperators function which will call underlying withdraw method if operator has some stake.
Impact: Medium
Operators won't be able to retrieve their staked LSTs, and the funds will be temporarily locked inside the OperatorStakingPool. One way to handle this issue in production would be for the owner to upgrade this implementation with a new one that withdraws all stuck funds. By reviewing past Withdraw events, the owner could redistribute the funds back to the operators.
Tools Used
Manual review, hardhat tests.
Recommendations
Inside _withdraw method after totalShares update, send amount of LSTs back to operator.
+ import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+ import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../core/interfaces/IStakingPool.sol";
contract OperatorStakingPool is Initializable, UUPSUpgradeable, OwnableUpgradeable {
using SafeERC20Upgradeable for IERC20Upgradeable;
+ using SafeERC20 for IERC20;
...
/**
* @notice Withdraws tokens
* @param _operator address of operator with withdraw for
* @param _amount amount to withdraw
**/
function _withdraw(address _operator, uint256 _amount) private {
uint256 sharesAmount = lst.getSharesByStake(_amount);
shareBalances[_operator] -= sharesAmount;
totalShares -= sharesAmount;
+ IERC20(address(lst)).safeTransfer(_operator, _amount);
emit Withdraw(_operator, _amount, sharesAmount);
}
