Using Solidity's transfer function has some notable shortcomings when the withdrawer is a smart contract, which can render ETH deposits impossible to withdraw. Specifically, the withdrawal will inevitably fail when:
The withdrawer smart contract does not implement a payable fallback function.
The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units.
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.
The sendValue function available in OpenZeppelin Contract’s Address library can be used to transfer the withdrawn Ether without being limited to 2300 gas units. Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the "Check-effects-interactions" pattern and using OpenZeppelin Contract’s ReentrancyGuard contract. For further reference on why using Solidity’s transfer is no longer recommended, refer to these articles:
Lines of code
https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/strategies/BkdEthCvx.sol#L77 https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/strategies/BkdEthCvx.sol#L93 https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/strategies/BkdEthCvx.sol#L117 https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/pool/EthPool.sol#L30 https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/vault/EthVault.sol#L29 https://github.com/code-423n4/2022-04-backd/tree/main/backd/contracts/vault/EthVault.sol#L37
Vulnerability details
Impact
Using Solidity's
transfer
function has some notable shortcomings when the withdrawer is a smart contract, which can render ETH deposits impossible to withdraw. Specifically, the withdrawal will inevitably fail when:The
sendValue
function available in OpenZeppelin Contract’s Address library can be used to transfer the withdrawn Ether without being limited to 2300 gas units. Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the "Check-effects-interactions" pattern and using OpenZeppelin Contract’s ReentrancyGuard contract. For further reference on why using Solidity’s transfer is no longer recommended, refer to these articles:Proof of Concept
strategies/BkdEthCvx.sol#L77\ strategies/BkdEthCvx.sol#L93\ strategies/BkdEthCvx.sol#L117\ pool/EthPool.sol#L30\
Note: If receiver is
ETHVault
contract andreceive()
function inETHVault
has additional logic, it could DoSvault/EthVault.sol#L29\ vault/EthVault.sol#L37
Tools Used
Manual review
Recommended mitigation steps
Use Solidity's low-level
call()
function or thesendValue
function available in OpenZeppelin Contract’s Address library to send Ether.