Golom contest - Treasure-Seeker's results

Lines of code

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

Vulnerability details

Vulnerability details

This is a classic Code4rena issue:

• https://github.com/code-423n4/2021-04-meebits-findings/issues/2
• https://github.com/code-423n4/2021-10-tally-findings/issues/20
• https://github.com/code-423n4/2022-01-openleverage-findings/issues/75

Impact

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

1. The claimer smart contract does not implement a payable function.
2. The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
3. The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call’s gas usage above 2300.

Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

Tools Used

VS Code

Recommended Mitigation Steps

It is recommended to use call instead of transfer. As a code reference, you may refer to the Openzeppelin Address.sendValue implementation.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L234-L239 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L298-L305 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L358-L365

Vulnerability details

Impact

ERC 721 and ERC 1155 transfers do not check for contract existence.

As per the solidity docs:

The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

This means that if any NFT contract (or its logic contract if upgradeable) is self destructed, Golom will not revert any operations and assume any transfers of those contracts are successful (even if the attacker does not own those NFTs).

Proof of Concept

A new NFT project is launched called Goblins and becomes very popular. However, their contract is self destructed because of a vulnerability.

There are 50 offers on Goblin NFTs on the Golom marketplace. An attacker accepts all those offers and makes 100 ETH (effectively stealing from the offerers since he/she does not own any Goblin NFTs). Because neither of the transfers check for contract existence, the transactions do not fail even though the attacker does not own any Goblin NFTs.

Tools Used

VS Code

Recommended Mitigation Steps

Marketplace contracts such as Opensea Seaport have a contract code existence check during transfers of ERC 721s and ERC 1155s.

It is advisable to add the same checks while transferring ERC 721s or ERC 1155s to be safe from such exploits.

Lines of code

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

Vulnerability details

Impact

At construction, the GolomTrader contract computes the domain separator using the network’s chainID, which is fixed at the time of deployment. In the event of a post-deployment chain fork, the chainID cannot be updated, and the signatures may be replayed across both versions of the chain.

Proof of Concept

Bob signs an order on the Ethereum mainnet. He signs the domain separator with a signature to sell an NFT. Later, Ethereum is hard-forked and as a result, there are two parallel chains with the same chain ID, and Eve can use Bob’s signature to match orders on the forked chain.

This exact vulnerability was also reported on LooksRare by Trail of Bits as a High Severity bug. Here's the report for your reference.

Tools Used

VS Code

Recommended Mitigation Steps

To mitigate this risk, if a change in the chainID is detected, the domain separator can be cached and regenerated. Alternatively, instead of regenerating the entire domain separator, the chainID can be included in the schema of the signature passed to the order hash.

Lines of code

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

Vulnerability details

Impact

When a listed NFT is bought using ETH (order type = 0), the msg.value check is a loose one (>=) but the excess msg.value is not refunded

Proof of Concept

This line of code ensures a min msg.value but the excess msg.value is not refunded anywhere in the fillAsk function.

This will cause a permanent loss of funds for the buyer.

Tools Used

VS Code

Recommended Mitigation Steps

Either refund the excess msg.value amount or change the check to a strict one (== rather than >=).

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L459-L461 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L313-L315 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/governance/Timlock.sol#L152-L154

Vulnerability details

Impact

Multiple contracts (GolomTrader, RewardDistributor and Timlock) have empty fallback and receive function implementations. This can cause a loss of funds as people may by mistake send the contract some eth, and will never be able to recover this ETH.

Since there is no purpose served by the fallback/receive functions, it is advisable to remove these functions.

Proof of Concept

Easy way to check is try sending any of these contracts some eth. It can now never be recovered.

Tools Used

VS Code

Recommended Mitigation Steps

Delete the fallback and receive functions