0xe4669da - DefaultBondStrategy::_deposit() could fail silently hence no tokens will be minted for Vault because return value of Vault::delegateCall is unchecked in DefaultBondStrategy::_deposit

[sherlock-admin3]

0xe4669da

Medium

DefaultBondStrategy::_deposit() could fail silently hence no tokens will be minted for Vault because return value of Vault::delegateCall is unchecked in DefaultBondStrategy::_deposit

Summary

Bond is an external smart contract and Bond::deposit function call could fail. DefaultBondStrategy::_deposit function call totally depends on external call Bond::deposit.

Vulnerability Detail

DefaultBondStrategy::_deposit is using Vault::delegateCall function, if Bond::deposit fails, it will fail silently. No tokens will be minted for Vault because the return value of Vault::delegateCall is unchecked in DefaultBondStrategy.

Scenario and transaction flow

• VaultConfigurator::depositCallback is set to DefaultBondstrategy
• Vault::deposit function is called by depositor
• DefaultBondStrategy::depositCallback is called
• DefaultBondStrategy::_deposit is called
• Vault::delegateCall is called return value is unchecked
• Vault gives delegatecall to DefaultBondModule
• DefaultBondModule calls Bond::deposit assuming failure of this function

Source

Code:

function _deposit() private {
    ...
@>  vault.delegateCall(
        address(bondModule),
        abi.encodeWithSelector(
            IDefaultBondModule.deposit.selector,
            data[j].bond,
            amount
        )
    );
}
    ...

Impact

No assets will be transferred to Bond smart contract. No bond tokens will be minted for Vault and failure will go unnoticed. This results into breakage of core functionality of mellow protocol.

Code Snippet

forge test --mt test_wy_DepositCallbackWithBonds --fork-url https://eth-mainnet.g.alchemy.com/v2/[api0-key]

Simply revert in DefaultBondMock.sol

function deposit(
        address recipient,
        uint256 amount
) external returns (uint256) {
    
    revert();

    ...
}

Add below test in DefaultBondStrategyTest.t.sol

Code

function test_wy_DepositCallbackWithBonds() external {
        
    Vault vault = new Vault("Mellow LRT Vault", "mLRT", admin);
    vm.startPrank(admin);
    vault.grantRole(vault.ADMIN_DELEGATE_ROLE(), admin);
    vault.grantRole(vault.OPERATOR(), operator);
    _setUp(vault);
    vm.stopPrank();
    _initialDeposit(vault);

    vm.startPrank(admin);

    VaultConfigurator configurator = VaultConfigurator(
        address(vault.configurator())
    );

    DefaultBondModule bondModule = new DefaultBondModule();
    DefaultBondStrategy strategy = new DefaultBondStrategy(
        strategyAdmin,
        vault,
        IERC20TvlModule(vault.tvlModules()[0]),
        bondModule
    );

    configurator.stageDepositCallback(address(strategy));
    configurator.commitDepositCallback();

    ManagedValidator validator = ManagedValidator(
        address(configurator.validator())
    );

    uint8 bondModuleRole = 1;

    validator.grantRole(address(strategy), bondModuleRole);
    validator.grantRole(address(vault), bondModuleRole);
    validator.grantContractRole(address(vault), bondModuleRole);
    validator.grantContractRole(address(bondModule), bondModuleRole);
    configurator.stageDelegateModuleApproval(address(bondModule));
    configurator.commitDelegateModuleApproval(address(bondModule));

    vault.grantRole(vault.OPERATOR(), address(strategy));

    vm.stopPrank();

    vm.startPrank(strategyAdmin);
    IDefaultBondStrategy.Data[]
        memory data = new IDefaultBondStrategy.Data[](2);
    data[0].bond = address(new DefaultBondMock(Constants.WSTETH));
    data[1].bond = address(new DefaultBondMock(Constants.WSTETH));
    data[0].ratioX96 = 0;
    data[1].ratioX96 = 2 ** 96;
    strategy.setData(Constants.WSTETH, data);
    vm.stopPrank();

    address depositor = address(bytes20(keccak256("depositor")));

    vm.startPrank(depositor);

    deal(Constants.WSTETH, depositor, 10 ether);
    IERC20(Constants.WSTETH).safeIncreaseAllowance(
        address(vault),
        10 ether
    );

    uint256[] memory amounts = new uint256[](3);
    amounts[0] = 10 ether;

    vault.deposit(depositor, amounts, 10 ether, type(uint256).max);
    vm.stopPrank();
    
}

Ran 1 test for tests/mainnet/unit/strategies/DefaultBondStrategyTest.t.sol:Unit
[PASS] test_wy_DepositCallbackWithBonds() (gas: 16723169)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 16.20s (14.80s CPU time)

Ran 1 test suite in 17.07s (16.20s CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tool used

Manual Review

Recommendation

In DefaultBondStrategy::_deposit check the return value of Vault::delegateCall function call because it returns bool success, if success == false then revert

[m-waqas88]

Why is this sponsor disputed? What is the reason for exclusion? I humbly request you to please elaborate?