joestakey - `BaseStrategyVault.redeemFromNotional()` can fail if `receiver` has a fallback function

[sherlock-admin]

joestakey

medium

BaseStrategyVault.redeemFromNotional() can fail if receiver has a fallback function

Summary

BaseStrategyVault.redeemFromNotional() can fail because of the use of the native ETH transfer method.

Vulnerability Detail

BaseStrategyVault.redeemFromNotional() transfers transferToReceiver to the receiver using the native transfer() function. The problem is that transfer() only allows the recipient to use 2300 gas. If the recipient uses more than that, transfers will fail. This can be the case if receiver is a smart contract wallet that performs some logic in its fallback() function - such as splitting payment to the wallet owners. In the future gas costs might change increasing the likelihood of that happening.

Proof Of Concept

Run the test in Transfer.t.sol from this private gist.

It tries to send ETH using a function via transfer() to two wallets:

• wallet1, which is a payment splitter wallet that transfers ETH to defined beneficiaries upon receiving ETH.
• wallet2, a simple smart contract wallet with no logic.

You can see that the transfer to wallet1 fails with an out of gas error, while the transfer to wallet2 works properly.

    ├─ [12002] PoCTransfer::withdraw(Wallet: [0x5b851f32f9ab7fb2c76480c608034a5ed1f16cfc], 1000000000000000000) 
    │   ├─ [2277] Wallet::fallback{value: 1000000000000000000}() 
    │   │   └─ ← "EvmError: OutOfGas"
    │   └─ ← "EvmError: Revert"
    └─ ← "EvmError: Revert"

Code Snippet

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/BaseStrategyVault.sol#L181

Tool used

Manual Review, Foundry

Recommendation

Use call() instead

-181:             if (transferToReceiver > 0) payable(receiver).transfer(transferToReceiver);
-182:             if (transferToNotional > 0) payable(address(NOTIONAL)).transfer(transferToNotional);
+181:             if (transferToReceiver > 0) payable(receiver).call{ value: transferToReceiver}(new bytes(0));
+182:             if (transferToNotional > 0) payable(address(NOTIONAL)).call{ value: transferToNotional}(new bytes(0));

Duplicate of #63