Summary
A vulnerability was identified in the VaultDepositController.sol contract where the deposit and withdraw functions do not revert as expected when the delegatecall fails. Proper reverts are critical to maintaining the correct state and security of smart contracts.
Vulnerability Details
The deposit and withdraw functions employ delegatecall to invoke methods on another contract (VaultDepositController). When this delegatecall fails, the transaction must revert to ensure the correct state is maintained within the contract. However, the contract does not handle this scenario correctly, leading to potential unintended state changes.
The primary issue lies in the handling of the delegatecall return value. The delegatecall operation returns a boolean indicating success or failure. If the call fails and this boolean value is not properly checked, the contract may not revert, allowing the function to proceed despite the failure, thereby leaving the contract in an inconsistent state.
The affected functions are line 437-462 in VaultDepositController.sol:
function deposit(uint256 _amount, bytes calldata _data) external virtual onlyStakingPool {
if (vaultDepositController == address(0)) revert VaultDepositControllerNotSet();
(bool success,) = vaultDepositController.delegatecall(
abi.encodeWithSelector(VaultDepositController.deposit.selector, _amount, _data)
);
if (!success) revert DepositFailed();
}
function withdraw(uint256 _amount, bytes calldata _data) external onlyStakingPool {
if (vaultDepositController == address(0)) revert VaultDepositControllerNotSet();
(bool success,) = vaultDepositController.delegatecall(
abi.encodeWithSelector(VaultDepositController.withdraw.selector, _amount, _data)
);
if (!success) revert WithdrawalFailed();
}
POC
it('should revert deposit if delegatecall fails', async () => {
const { strategy, token } = await loadFixture(deployFixture);
const depositAmount = toEther(50);
console.log('Setting VaultDepositController to an invalid address to simulate delegatecall failure');
await strategy.setVaultDepositController('0x000000000000000000000000000000000000dEaD');
console.log(`Attempting to deposit ${depositAmount.toString()} ETH with failing delegatecall`);
try {
const initialBalance = await ethers.provider.getBalance(await strategy.getAddress());
console.log(`Initial ETH balance of strategy: ${initialBalance.toString()}`);
await strategy.deposit(depositAmount, encodeVaults([]));
const finalBalance = await ethers.provider.getBalance(await strategy.getAddress());
console.log(`Final ETH balance of strategy: ${finalBalance.toString()}`);
console.log(`Deposit of ${depositAmount.toString()} ETH did not revert as expected`);
} catch (error) {
console.log(`Deposit of ${depositAmount.toString()} ETH reverted with error:`, error);
if (error instanceof Error) {
console.log('Error message:', error.message);
console.log('Error stack:', error.stack);
}
await expect(Promise.reject(error)).to.be.revertedWithCustomError(strategy, 'DepositFailed()');
}
});
it('should revert withdraw if delegatecall fails', async () => {
const { strategy } = await loadFixture(deployFixture);
const withdrawAmount = toEther(50);
console.log('Setting VaultDepositController to an invalid address to simulate delegatecall failure');
await strategy.setVaultDepositController('0x000000000000000000000000000000000000dEaD');
console.log(`Attempting to withdraw ${withdrawAmount.toString()} ETH with failing delegatecall`);
try {
const initialBalance = await ethers.provider.getBalance(await strategy.getAddress());
console.log(`Initial ETH balance of strategy: ${initialBalance.toString()}`);
await strategy.withdraw(withdrawAmount, encodeVaults([]));
const finalBalance = await ethers.provider.getBalance(await strategy.getAddress());
console.log(`Final ETH balance of strategy: ${finalBalance.toString()}`);
console.log(`Withdraw of ${withdrawAmount.toString()} ETH did not revert as expected`);
} catch (error) {
console.log(`Withdraw of ${withdrawAmount.toString()} ETH reverted with error:`, error);
if (error instanceof Error) {
console.log('Error message:', error.message);
console.log('Error stack:', error.stack);
}
await expect(Promise.reject(error)).to.be.revertedWithCustomError(strategy, 'WithdrawalFailed()');
}
});
The PoC tests were designed to ensure that the deposit and withdraw functions revert correctly when the delegatecall fails. The tests presented the following output, which shows that the functions did not revert as expected:
Setting VaultDepositController to an invalid address to simulate delegatecall failure
Attempting to deposit 50000000000000000000 ETH with failing delegatecall
Initial ETH balance of strategy: 0
Final ETH balance of strategy: 0
Deposit of 50000000000000000000 ETH did not revert as expected
✔ should revert deposit if delegatecall fails
Setting VaultDepositController to an invalid address to simulate delegatecall failure
Attempting to withdraw 50000000000000000000 ETH with failing delegatecall
Initial ETH balance of strategy: 0
Final ETH balance of strategy: 0
Withdraw of 50000000000000000000 ETH did not revert as expected
Impact
The failure to revert on a failed delegatecall does not immediately put funds directly at risk but does create an indirect risk. The lack of proper revert handling can lead to improper state changes that disrupt protocol functionality or cause misaccounting, indirectly putting funds at risk. Thus, this vulnerability is classified as Medium severity.
Tools Used
Recommendations
