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

Lack of Access Control on `VaultDepositController:withdraw` Leading to Loss of Funds from Vaults

0xbeastboy

Github

Summary

The VaultDepositController:withdraw function lacks adequate access control, which allows anyone to call the function and withdraw funds from the vaults. This absence of a proper access control mechanism creates a significant risk of unauthorized withdrawals, compromising the integrity of the funds stored in the vaults.

Vulnerability Details

The withdraw function in the VaultDepositController contract is marked as external and lacks a proper access control mechanism that limits its execution to authorized entities. In its current state, any external account can invoke this function to withdraw funds from the vaults, which poses a significant risk to the security of the funds managed by the contract.

The expected behavior for the withdraw function is that it should only be callable by a designated contract, such as the StakingPool, which acts as the central authority for managing withdrawals. This approach is consistent with the access control mechanism implemented in the StakingPool and PriorityPool contracts, where functions are restricted using modifiers like onlyStakingPool.

Currently, the withdraw function does not have any access control implemented, which means any external account can call this function and potentially withdraw funds from the vaults without restriction.

Example Scenario

  • An attacker discovers that the withdraw function is not restricted by any access control mechanism.

  • The attacker calls the withdraw function with an amount and data of their choice.

  • The function executes the withdrawal logic and transfers funds to the attacker's address, leading to the loss of funds from the vaults.

/**
* @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 {
if (!fundFlowController.claimPeriodActive() || _amount > totalUnbonded)
revert InsufficientTokensUnbonded();
​
GlobalVaultState memory globalState = globalVaultState;
uint64[] memory vaultIds = abi.decode(_data, (uint64[]));
VaultGroup memory group = vaultGroups[globalState.curUnbondedVaultGroup];
​
// withdrawals must continue with the vault they left off at during the previous call
if (vaultIds[0] != group.withdrawalIndex) revert InvalidVaultIds();
​
uint256 toWithdraw = _amount;
uint256 unbondedRemaining = totalUnbonded;
(uint256 minDeposits, ) = getVaultDepositLimits();
​
for (uint256 i = 0; i < vaultIds.length; ++i) {
// vault must be a member of the current unbonded group
if (vaultIds[i] % globalState.numVaultGroups != globalState.curUnbondedVaultGroup)
revert InvalidVaultIds();
​
group.withdrawalIndex = uint64(vaultIds[i]);
IVault vault = vaults[vaultIds[i]];
uint256 deposits = vault.getPrincipalDeposits();
​
if (deposits != 0 && vault.claimPeriodActive() && !vault.isRemoved()) {
if (toWithdraw > deposits) {
vault.withdraw(deposits);
unbondedRemaining -= deposits;
toWithdraw -= deposits;
} else if (deposits - toWithdraw > 0 && deposits - toWithdraw < minDeposits) {
// cannot leave a vault with less than minimum deposits
vault.withdraw(deposits);
unbondedRemaining -= deposits;
break;
} else {
vault.withdraw(toWithdraw);
unbondedRemaining -= toWithdraw;
break;
}
}
}
​
uint256 totalWithdrawn = totalUnbonded - unbondedRemaining;
​
token.safeTransfer(msg.sender, totalWithdrawn);
​
totalDeposits -= totalWithdrawn;
totalPrincipalDeposits -= totalWithdrawn;
totalUnbonded = unbondedRemaining;
​
group.totalDepositRoom += uint128(totalWithdrawn);
vaultGroups[globalVaultState.curUnbondedVaultGroup] = group;
}

Impact

An attacker or unauthorized user could call the withdraw function to withdraw any amount from the vaults, resulting in a direct financial loss. The uncontrolled withdrawal of funds could lead to the depletion of the vault’s liquidity, preventing legitimate users or contracts from performing their operations.

Tools Used

Manual Review

Recommendations

Add a new modifier to the VaultDepositController contract that ensures only the StakingPool contract can call the withdraw function.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
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.