Golom contest - StErMi's results

Judge has assessed an item in Issue #330 as Medium risk. The relevant finding follows:

GolomTrader is not using ERC721 safeTransferFrom when transferring tokens All the functions that fill orders are not using safeTransferFrom when the owner of the NFT transfer the token to the user or contract that has made the purchase.

If the taker is a contract and has not implemented the needed functions, that token will be stuck on the contract itself forever.

Consider using safeTransferFrom as well for the ERC721.

ERC721 Order of orderType 2 for the same TokenID can be filled multiple times by maker via fillCriteriaBid function forcing the taker to buy it again

Important note

This would have been a mid/high-level finding normally but as far as I understand by speaking with the developer they do not allow creating Order with orderType 2 from the frontend side with the taker that can specify a specific tokenID of a collection.

Impact

The current implementation of the fillCriteriaBid allow a maker to fill multiple time the same order of type orderType 2 if the order is of type isERC721 and the tokenAmt is greater than 1.

This mean that a taker would be "forced to buy back" the NFT even if you don't want to.

This could allow a maker to force a taker to buy again the same NFT against his/her will if the order has not expired yet.

Let's make an example:

1. taker convert 100 ETH to WETH
2. taker approve the Golom marketplace for the whole amount
3. taker create an order for an ERC721 NFT with orderType = 2, tokenAmt = 2, isERC721 = true, tokenId = 1 and totalAmt = 10 ETH
4. maker see the order and sell the NFT for by calling fillCriteriaBid
5. The NFT depreciate a lot in value, and now its price is only 1 ETH
6. taker sell it back to maker for that price
7. maker call again fillCriteriaBid on the same taker's order
8. taker is forced by the maker to buy again the NFT for a higher price compared to what is worth

Proof of Concept

Here's the hardhat test used to replicate the above scenario: https://gist.github.com/StErMi/fbd87341153f2552519ad189188b67d1

Recommended Mitigation Steps

Prevent the taker of the order to create orders of type ERC721 that have tokenAmt != 1 otherwise when the token is filled the first update filled[hashStruct] = order.tokenAmt if the order isERC721 is true

fillCriteriaBid allow selling a tokenID different compared to the one specified in the Order, allowing the maker to sell a low-value token but forcing the taker to pay for a higher value one

Important note

This would have been a mid/high-level finding normally but as far as I understand by speaking with the developer they do not allow creating Order with orderType 2 from the frontend side with the taker that can specify a specific tokenID of a collection.

Impact

Note: from the same collections tokenIDs could have different market value because of rarity, properties, graphics and so on.

The current implementation of the fillCriteriaBid allow a maker to fill an order with a more valuable tokenID (higher cost for the taker) but specifying the tokenID of a lower value one.

Let's make an example:

taker want to purchase 2 different tokenID from the same collection. One tokenID values 1 ETH, the other value 50 ETH. Both of those are owned by maker.

1. taker convert 100 ETH to WETH
2. taker approve the Golom marketplace for the whole amount
3. taker create an order for NFT.tokenID 1 with BID price == 50 ETH
4. taker create an order for NFT.tokenID 2 with BID price == 1 ETH
5. maker own both of those NFTs but will exploit the protocol by selling the order that contains the info of the valuable NFT but specifying the ID of the less valuable NFT when calling fillCriteriaBid
6. taker will receive the less valuable NFT but will pay the cost of the most valuable one

Proof of Concept

Here's the hardhat test used to replicate the above scenario: https://gist.github.com/StErMi/1310ef04b72aaeab7b5d08d8fac69733

validateOrder will never return (0, hashStruct, 0)

The current implementation of validateOrder has the current snippet of code

require(signaturesigner == o.signer, 'invalid signature');
if (signaturesigner != o.signer) {
    return (0, hashStruct, 0);
} 

If the signaturesigner is not equal to the o.signer the function will directly revert, this mean that it's impossible for the code to reach the if branch that would execute return (0, hashStruct, 0);

fillAsk allow the taker to send more ETH than needed to fill the order. Those ETHs will be stuck forever

The current implementation of fillAsk does not check that msg.value strictly match the required amount of ETH needed to fill the order.

The current check implemented is require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');

This allows the user to send more ETH than needed. The difference between msg.value and the amount had to fill the order and pay for the fees will be stuck in the GolomTrader contract without a way to withdraw them or repay back the taker.

GolomTrader WETH can be defined as constant

The immutable state variable WETH could be defined as constant instead of immutable

Note: this is not part of gas optimization just because it's already optimized, but for clarity it should be defined as a constant given that this variable value will never change.

GolomTrader governance state variable is never used

This variable is passed as a constructor input parameter, but is never used inside the code.

GolomTrader is not using ERC721 safeTransferFrom when transferring tokens

All the functions that fill orders are not using safeTransferFrom when the owner of the NFT transfer the token to the user or contract that has made the purchase.

If the taker is a contract and has not implemented the needed functions, that token will be stuck on the contract itself forever.

Consider using safeTransferFrom as well for the ERC721.

GolomTrader fee paid to the distributor could be prone to rounding errors

In general, the fee paid to the distributor is calculated as ((o.totalAmt * 50) / 10000) * amount

• If the o.totalAmt * 50 is less than 10_000 the result will be 0
• the multiplication by amount should be placed before the division to prevent rounding error

GolomToken is not following Checks-Effects-Interactions Pattern

Both mintAirdrop and mintGenesisReward are not following CEI pattern.

They should update the internal state before minting tokens

function mintAirdrop(address _airdrop) external onlyOwner {
    require(!isAirdropMinted, 'already minted');
+    isAirdropMinted = true;
    _mint(_airdrop, 150_000_000 * 1e18);
-    isAirdropMinted = true;
}

function mintGenesisReward(address _rewardDistributor) external onlyOwner {
    require(!isGenesisRewardMinted, 'already minted');
+    isGenesisRewardMinted = true;
    _mint(_rewardDistributor, 62_500_000 * 1e18);
-    isGenesisRewardMinted = true;
}

Remove hardhat/console.sol import from RewardDistributor

Remove the hardhat/console.sol import from the contract before deploying to production.

VoteEscrowDelegation delegate is shadowing a state variable

tokenId input parameter is shadowing the contract state variable tokenId. Quoting from SWC-119 Shadowing State Variable

Solidity allows for ambiguous naming of state variables when inheritance is used. Contract A with a variable x could inherit contract B that also has a state variable x defined. This would result in two separate versions of x, one of them being accessed from contract A and the other one from contract B. In more complex contract systems this condition could go unnoticed and subsequently lead to security issues.

Consider renaming the input parameter from tokenId to _tokenId

GolomTrader should prevent the owner of the order to call it when the Order has been already filled

The cancelOrder function is only checking that the caller is also the owner of the Order, but is not checking if the order has been already cancelled or filled.

This could create two different problems:

• cancelled order can be cancelled again and again, spamming the OrderCancelled event
• OrderCancelled events can be emitted even if the order has been filled and the OrderFilled event has been emitted

Consider having two separate state for the order and prevent cancelling an order that has been cancelled or cancelling an order that has been already filled