Golom contest - RustyRabbit's results

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L381 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L390 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L397

Vulnerability details

Impact

When an order for multiple ERC115 NFTs is filled via either fillBid() or fillCriteriaBid() with an amount >1 the calculation of the protocolfee is first calculated to be 5% of the price of each NFT multiplied by the amount of NFTs to be filled specified in the call. Then however when the total amount to be sent to the seller of the NFT is calculated the protocolfee is again multiplied by the amount as calculation to be deducted from what is to be sent to the seller. This means the seller of the NFT isn't getting the correct amount and the difference is stuck in the contract.

Proof of Concept

protocolfee is calculated on line 381 to already take into account the ammount of ERC1155 tokens being exchanged. Then at line 390 and 397 the protocolfee is being deducted from order.totalAmt (which is the amount of ETH per NFT) and then multiplied again by the amount of tokens being exchanged. This means the taker is getting less than expected and this ETH is then stuck in the contract as only the correct amount is being sent (on line 384) to the distributor.

Tools Used

Manual detection

Recommended Mitigation Steps

Perform the correct calculation at line 381 but be sure to then multiply the protocolfee by the amount of NFTs transfered to pay the correct amount of ETEH to the distributor in line 384.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L175

Vulnerability details

Impact

Validation of signatures is done against the chainid when the contract was deployed. This means that the contract will never work on another chainid.

The goal of including the chainid in the signature (EIp155 and EIP712) is to make sure that the signed data is not replayed on a chain it wasn't intended to be used by the signer, not limit the usage of the deployed contract on a fork of the original chain.

Of course if this is the intended design this point is moot, but be aware that there is no guarantee, however unlikely, that future forks will keep the same chainid. This could be due to technical or political reasons.

Risk rating is only set to medium because the chance the current chainid is not continued at all is relatively small. So the contracts will still keep running if even on a chain the project doesn't support. Also this only involves order treatment and doesn't involve locked funds.

Proof of Concept

NA

Tools Used

NA

Recommended Mitigation Steps

Use the OpenZeppelin draft-EIP712.sol which allows the contract to work (with signature intended for the new chainid) on the new chainid. If the team wants control over which chainId it wants to support this could be done by a governance function allowed to set the chainId.

Lines of code

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

Vulnerability details

Impact

The require statement allows msg.value to be greater than the total amount payed to the seller, distributor, exchange, referrer, the 50 basis point fee plus the extra payment. So when the taker sends to much ETH by mistake (or signature request by a malicious exchange/actor) it will increase the ETH balance of the contract and there is no way to retrieve it.

Proof of Concept

NA

Tools Used

NA

Recommended Mitigation Steps

Either require the exact amount or provide a refund to the taker for whatever amount of ETH is sent to much. Alternatively provide a function to retrieve any stuck ETH.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L459 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L461 https://github.com/code-423n4/2022-07-golom/blob/f6711139d1430316ec7faddcf8e47aa3098762d9/contracts/rewards/RewardDistributor.sol#L313 https://github.com/code-423n4/2022-07-golom/blob/f6711139d1430316ec7faddcf8e47aa3098762d9/contracts/rewards/RewardDistributor.sol#L315

Vulnerability details

Impact

Fallback and receive functions mean people can accidentally send ETH to the contract which will be stuck there.

Proof of Concept

NA

Tools Used

NA

Recommended Mitigation Steps

GolomTrader.sol needs to be able to receive ETH when withdrawing from the WETH contract which uses a transfer with limited gas, so it can't do any triage in the recieve() function. An extra function to be able to retrieve any excess ETH under control of governance would be a possible solution here.

RewardDistributor.sol needs to be able to receive ETH from the GolomTrader.sol when forwarding the protocolfee, but this could better be done buy a specialized function instead of a general receive function.

Remove the fallback() functions as they don't seem to be necessary.