Escher contest - btk's results

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L85-L86 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L105 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPrice.sol#L66 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L74 https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEdition.sol#L67

Vulnerability details

M-1 call() should be used instead of transfer()

Impact

Note: C4 declared this finding as an unsafe ERC20 operation in the c4udit output, which is not true (The Escher protocol does not support erc20 tokens). This is a deprecated transfer() that could significantly damage the protocol and the users.

If saleReceiver and feeReceiver where smart contracts and one of the scenarios described in the POC occurred, neither of them will be able to withdraw their funds, and they (funds) will be frozen in the contract, resulting in a loss of funds for both users and the protocol.

Proof of Concept

The use of the deprecated transfer() function for a payable address will certainly make the transaction fail when:

• The claimer smart contract does not implement a payable fallback function.

• The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.

• The claimer 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.

• Some multisig wallets might also require higher gas than 2300.

A more detailed explanation can be found here.

Lines of code:

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L85-L86

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L105

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using .call() instead with the CEI pattern implemented correctly. .

        (bool success, ) = payable(msg.sender).call{value: owed}("");
        require(success, "Call failed");

        (bool success, ) = ISaleFactory(factory).feeReceiver().call{value: address(this).balance / 20}("");
        require(success, "Call failed");  

M-2 Use safeMint instead of mint for ERC721

Impact

Users could lost their NFTs.

The msg.sender will be minted an nft when buy is called.

However, if msg.sender is a contract address that does not support ERC721, the NFT can be frozen in the contract forever.

As per the documentation of EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

Ref: https://eips.ethereum.org/EIPS/eip-721

As per the documentation of ERC721.sol by Openzeppelin:

Ref: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L274-L285

Proof of Concept

Lines of code:

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/FixedPrice.sol#L66

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L74

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/OpenEdition.sol#L67

Tools Used

Manual Review

Recommended Mitigation Steps

Use safeMint instead of mint to check received address support for ERC721 implementation.

    function _safeMint(
        address to,
        uint256 tokenId,
        bytes memory data
    ) internal virtual {
        _mint(to, tokenId);
        require(
            _checkOnERC721Received(address(0), to, tokenId, data),
            "ERC721: transfer to non ERC721Receiver implementer"
        );
    }