Escher contest - __141345__'s results

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L109 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L85-L86 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L105 https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/OpenEdition.sol#L92

Vulnerability details

Impact

When operations use a wrapped native token, the withdraw is being handled with a payable.transfer() method.

When withdrawing and refund extra ETH, the ETHRegistrarController contract uses Solidity’s transfer() function.

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.

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. 

Proof of Concept

File: src/minters/FixedPrice.sol
109:         ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);
110:         selfdestruct(_sale.saleReceiver);

File: src/minters/LPDA.sol
85:             ISaleFactory(factory).feeReceiver().transfer(fee);
86:             temp.saleReceiver.transfer(totalSale - fee);

105:         payable(msg.sender).transfer(owed);

File: src/minters/OpenEdition.sol
92:         ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);

References:

The issues with transfer() are outlined here

For further reference on why using Solidity’s transfer() is no longer recommended, refer to these articles.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised, reference.