Sturdy contest - hake's results

Lines of code

GeneralVault.sol#L100

Vulnerability details

Impact

User can have his collateral burned (LendingPool.sol#L324) and not receive funds from the Vault when calling withdrawCollateral.

Proof of Concept

LidoVault.sol#L140

(bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');
return receivedETHAmount;
require(sent, Errors.VT_COLLATERAL_WITHDRAW_INVALID);

The call boolean check is unreachable because it only occurs after the function has already ceased execution with return above.

Scenario:

• User calls withdrawCollateral.
• Collateral is burned.
• The call in _withdrawFromYieldPool fails but doesn't revert due to return placement.
• withdrawAmount still reflects receivedETHAmount.
• All checks pass without user receiving.

Recommended Mitigation Steps

Please change check position so it is reachable:

(bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');
require(sent, Errors.VT_COLLATERAL_WITHDRAW_INVALID);
return receivedETHAmount;

LOW

L-01: Outdated compiler version (pragma solidity 0.6.12)

Older compilers might be susceptible to some bugs.

I recommend changing the solidity version pragma to the latest version to enforce the use of an up-to-date compiler.

A list of known compiler bugs and their severity can be found here: https://etherscan.io/solcbuginfo

L-02: Unsafe approve

YieldManager.sol#L221

The ERC20.approve() function returns a boolean value indicating success. This parameter needs to be checked for success.

I recommend using OpenZeppelin’s safeApprove function that handle the return value check as it was done it the other contracts.

NON-CRITICAL

N-01: safeApprove has been deprecated

ConvexCurveLPVault.sol#L158

LidoVault.sol#L102

Consider using safeIncreaseAllowance and safeDecreaseAllowance instead.

REFERENCE

GAS

G-01: for loop gas optimization

Gas could be saved by:

for (uint256 i = 0; i < extraRewardsLength; i++) {
    address _extraReward = IConvexBaseRewardPool(baseRewardPool)
        .extraRewards(i);
    address _rewardToken = IRewards(_extraReward).rewardToken();
    _transferYield(_rewardToken);
}

• Not initializing variable to default value of zero
• Using a prefix (++i) instead of a postfix (i++)
• Unchecking increment count

Example:

for (uint256 i; i < extraRewardsLength;) {
    address _extraReward = IConvexBaseRewardPool(baseRewardPool)
        .extraRewards(i);
    address _rewardToken = IRewards(_extraReward).rewardToken();
    _transferYield(_rewardToken);

    unchecked { ++i; }
}