Golom contest - AuditsAreUS's results

Lines of code

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

Vulnerability details

Impact

GolomTrader.validateOrder() does not check o.signer != address(0) as a result it is possible to create orders with o.signer == address(0). Therefore we may always create orders with the zero address.

The reason using the zero address as the signer succeeds is the ecrecover() used on #176 will return address(0) if the provided signature is invalid.

This issue is rated as medium as although it is undesirable to create orders on behalf of addresses that we do not have the key for it is unlikely that we will be able to transfer funds from the zero address and thus this will only succeed for orders which transfer a zero amount of tokens.

Proof of Concept

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

        address signaturesigner = ecrecover(hash, o.v, o.r, o.s);
        require(signaturesigner == o.signer, 'invalid signature');
        if (signaturesigner != o.signer) {
            return (0, hashStruct, 0);
        }

Recommended Mitigation Steps

To resolve this issue consider updating the code listed above as follows.

        if (signaturesigner == address(0) || signaturesigner != o.signer) {
            return (0, hashStruct, 0);
        }

Lines of code

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

Vulnerability details

Impact

It is possible to send a higher msg.value than is required to fillAsk(). The excess value that is sent will be permanently locked in the contract.

Proof of Concept

There is only one check over msg.value and it is that it's greater than o.totalAmt * amount + p.paymentAmt. As seen in the following code snippet from #217.

        require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');

The issue here is that the contract will only ever spend exactly o.totalAmt * amount + p.paymentAmt. Hence if msg.value is greater than this then the excess value will be permanently locked in the contract.

Recommended Mitigation Steps

To avoid this issue consider enforcing a strict equality.

        require(msg.value == o.totalAmt * amount + p.paymentAmt, 'mgmtm');

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.