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

Missing access controls in the `VaultDepositController::withdraw` function, which means that anyone could call it.

0xrolko

Relevant GitHub Links

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/base/VaultControllerStrategy.sol#L107

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/base/VaultControllerStrategy.sol#L111

Summary

No access control was apply to the VaultDepositController::withdraw function.This means that anyone can call it, and with any type of call.

Vulnerability Details

The VaultDepositController::withdraw function should only be called by the VaultControllerStrategy contract and only using the delegatecall as specified in the documentation:

/**
* @notice Withdraws tokens from vaults and sends them to staking pool
@> * @dev called by VaultControllerStrategy using delegatecall
* @param _amount amount to withdraw
* @param _data encoded vault withdrawal order
*/
function withdraw(uint256 _amount, bytes calldata _data) external {
/// ... The function code
​
}

We can see this `delegatcall` in action in the abstract contract `VaultControllerStrategy` :

function withdraw(uint256 _amount, bytes calldata _data) external onlyStakingPool {
// lack of non zero check
if (vaultDepositController == address(0)) revert VaultDepositControllerNotSet();
​
@> (bool success, ) = vaultDepositController.delegatecall(
abi.encodeWithSelector(VaultDepositController.withdraw.selector, _amount, _data)
);
​
if (!success) revert WithdrawalFailed();
}

We see that the VaultDepositController::withdraw is called via delegatecall by the VaultControllerStrategy and that msg.sender is the staking pool as explained in the documentation, which is fine. But the problem is that no restrictions have been applied to the VaultDepositController::withdraw target function to ensure that the msg.sender is always the staking pool and that the call is always the delegatecall. This means that the VaultDepositController::withdraw function can also be called directly from any address other than the address of staking pool.

Impact

Unexpected behavior. And If any external user or contract is able to call VaultDepositController::withdraw function, it could lead to undesirable outcomes, such as unauthorized withdraw or Unexpected behavior issues.

Tools Used

Manual analysis.

Recommendations

/**
* @notice Withdraws tokens from vaults and sends them to staking pool
* @dev called by VaultControllerStrategy using delegatecall
* @param _amount amount to withdraw
* @param _data encoded vault withdrawal order
*/
function withdraw(uint256 _amount, bytes calldata _data) external
+ onlyStakingPool
{
+ require(address(this) != msg.sender, "Direct calls not allowed");
/// ... The function code
​
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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