Escher contest - Tutturu's results

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/FixedPrice.sol#L107-L111 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

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

1. The receiver smart contract does not implement a payable function.
2. The receiver smart contract does implement a payable fallback that uses more than 2300 gas.
3. The receiver smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.
4. The receiver smart contract is a multisig wallet that uses more than 2300 gas

This can lead to permanent lock-up of funds for both the factory feeReceiver as well as the creator of the sale.

Proof of Concept

1. Alice deploys a FixedPrice sale and manages to collect 10 ETH through the sale. The feeReceiver is a multisig wallet.
2. Bob tries to buy the last NFT, triggering _end()
3. The _end() function tries to transfer the protocol share to the feeReceiver ISaleFactory(factory).feeReceiver().transfer(address(this).balance / 20);
4. The function reverts due to insufficient gas

This will DoS the function leading to locking up the last NFT as well as all of the funds inside the contract. Thankfully, this could be mitigated by changing the feeReceiver in the factory contract, which is also the case for OpenEdition.sol.

However, if Alice were to deploy an LPDA contract and set her own saleReceiver address to a smart contract, this could lead to a longer (potentially permanent) lock-up of funds since Alice has no way of updating the saleReceiver address. The only way to resolve this issue would be to use Optional Access Lists, which would work for multisig wallets, but is not guaranteed to work for all types of smart contracts.

Tools Used

Foundry, manual review

Recommended Mitigation Steps

I recommend using .call() instead of .transfer() and adding a nonReentrant modifier to any function that replaces .transfer() with .call() that could potentially be re-entered. One such function is finalize() in OpenEdition.sol since it could be used to drain (almost) the entire balance to the feeReceiver() by reentering the function multiple times.