Potential Overlap Between ERC20 and ERC721 Allows For Transferring of ERC20 Tokens Such As WETH

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L236 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L361 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L301

Vulnerability details

Impact

There is overlap between the function signature of ERC20 transferFrom(address,address,uint256) and ERC721 transferFrom(address,address,uint256). The impact of this is that it's possible for users to transfer ERC20 tokens instead of ERC721.

This poses a threat as signers may have approved WETH to transfer funds. The threat occurs if o.collection is then set as WETH address. An unwitting user could accidently transfer WETH instead of an NFT to a user.

The impact is rated as Medium rather than High as although there is loss of funds the user would have to either sign or accept an Order which has a ERC20 address as the collection field.

Proof of Concept

fillAsk()

            ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);

fillBid()

            ERC721 nftcontract = ERC721(o.collection);
            nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);

fillCriteriaBid()

            ERC721 nftcontract = ERC721(o.collection);
            nftcontract.transferFrom(msg.sender, o.signer, tokenId);

Recommended Mitigation Steps

Consider using ERC721.safeTransferFrom() instead of ERC721.transferFrom(). This will change the function signature and therefore prevent overlap between ERC20 and ERC721 transfer functions.

[0xsaruman]

duplicate of https://github.com/code-423n4/2022-07-golom-findings/issues/342

[dmvt]

This is not a duplicate of #342. The maximum compromise here is an amount of WEI equal to the tokenId. I think this is an improbable scenario, but I'll leave it in as low risk. Downgraded to QA.