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

Lack of access controls in the `VaultDepositController::deposit` 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#L83

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

Summary

There is a possibility that the VaultDepositController::deposit function could be called by an external contract or any other address without using delegatecall.

Vulnerability Details

According to the documentation, function VaultDepositController::deposit should only be called by VaultControllerStrategy using delegatecall. however, no check has been made to ensure that this function is only called only using delegatecall and by no other address than address(VaultControllerStrategy). This means that any address can call this function and by direct calls.

/**
* @notice Deposits tokens from the staking pool into vaults
@> * @dev called by VaultControllerStrategy using delegatecall
* @param _amount amount to deposit
* @param _data encoded vault deposit order
*/
@> function deposit(uint256 _amount, bytes calldata _data) external {
/// ... The rest of code
}

In the VaultControllerStrategy abstract contract, we can see that access control is applied to the VaultControllerStrategy::deposit function and that the VaultDepositController::deposit function is called via delegatecall :

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

So the VaultDepositController::deposit function will be executed in the context of the staking pool as desired. But no restrictions have been placed on this function to ensure that it is only called by delegatecall and only by the staking pool.

Impact

Unexpected behavior. And If any external user or contract is able to call deposit directly and transfer tokens, it could lead to undesirable outcomes, such as unauthorized deposits or incorrect token transfers.

Tools Used

Manual review.

Recommendations

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

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!