Usage of deprecated transfer() can result in revert, BaseStrategyVault.sol usage of payable(address).transfer, not _to.call{value: msg.value}("") when handling ETH transfer

Summary

The function redeemFromNotional in BaseStrategyVault.sol is used to redeem the notional ETH payment.

Vulnerability Detail

in the function redeemFromNotional,

Usage of deprecated transfer() can result in revert,

      if (_UNDERLYING_IS_ETH) {
            if (transferToReceiver > 0) payable(receiver).transfer(transferToReceiver);
            if (transferToNotional > 0) payable(address(NOTIONAL)).transfer(transferToNotional);
        } else {
            if (transferToReceiver > 0) _UNDERLYING_TOKEN.checkTransfer(receiver, transferToReceiver);
            if (transferToNotional > 0) _UNDERLYING_TOKEN.checkTransfer(address(NOTIONAL), transferToNotional);
        }

transfer() uses a fixed amount of gas, which was used to prevent reentrancy. However this limit your protocol to interact with others contracts that need more than that to process the transaction.

Specifically, the withdrawal will inevitably fail when: 1.The withdrawer smart contract does not implement a payable fallback function. 2.The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units. 3.The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

Impact

transfer() uses a fixed amount of gas, which can result in revert. https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Code Snippet

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

Tool used

Manual Review

Recommendation

Use call instead of transfer(). Example: (bool succeeded, ) = _to.call{value: _amount}(""); require(succeeded, "Transfer failed.");

Duplicate of #63

sherlock-audit / 2022-09-notional-judging

ctf_sec - Usage of deprecated transfer() can result in revert, BaseStrategyVault.sol usage of payable(address).transfer, not _to.call{value: msg.value}("") when handling ETH transfer #94