Golom contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

You should use call() instead of transfer(). If the recipient is a contract it might use more than 2300 gas which will make the transaction fail if the ETH was sent with transfer().

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

The functions follow the check effects interactions pattern. I couldn't find any reentrancy issues.

Proof of Concept

Tools Used

none

Recommended Mitigation Steps

Use call() instead.

Lines of code

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

Vulnerability details

Impact

You're already using safeTransferFrom() for ERC1155 contracts but not for ERC721 ones. You allow scenarios where the user might transfer the bought token to an address that can't handle them. The tokens would be locked up. Because of that I'd rate the issue as MED.

The possibility of reentrancy has to be evaluated. I couldn't find any reentrancy issues until now. The check effects interactions pattern is used within the relevant functions.

Proof of Concept

Tools Used

none

Recommended Mitigation Steps

Use safeTransferFrom() for ERC721 contracts.

Lines of code

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

Vulnerability details

Impact

In GolomTrader.fillAsk() the buyer is allowed to send more ETH than necessary to fulfill the order. The contract doesn't have any way of using the excess amount. The funds will be locked up in the contract.

This issue depends on the buyer making a mistake and sending more than necessary. Thus, I'd rate it as MED.

Proof of Concept

The contract only transfers ETH when payEther() is called. But, that function is only callable within a function that handles the orders. There it only works with the ETH that was part of that specific order. It never accesses excess ETH.

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

Tools Used

none

Recommended Mitigation Steps

Only allow the buyer to send the exact amount of ETH that should be spend: require(msg.value == o.totalAmt * amount + p.paymentAmt, 'mgmtm');

Report

• Low
  • L-01: royalty fees can be higher than 10%
  • L-02: hardcoded WETH address might lead to issues in the future
  • L-03: set the startTime in RewardDistributor through the constructor
• Non-Critical
  • N-01: emit an event when changing the contract's configuration

Low

L-01: royalty fees can be higher than 10%

According to the docs, the royalty fees should only be able to go up to 10%, see https://docs.golom.io/features/lowest-fees#royalty-fees. But, the contract allows the seller to choose the value freely.

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

There's no validation for the prePayment value.

Either update the docs or implement the royalty fees as specified.

L-02: hardcoded WETH address might lead to issues in the future

According to the docs, the protocol is supposed to be launched on other chains as well in the future. The address of the WETH contract is different on other chains. If you don't update the value before deployment you might run into some small isseus.

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

https://docs.golom.io/faqs

L-03: set the startTime in RewardDistributor through the constructor

There'll always be an issue that will make you postpone the launch. Setting the value through the constructor will save you the work of modifying the value multiple times.

Non-Critical

N-01: emit an event when changing the contract's configuration

It allows third parties to track those changes efficiently.

e.g.

• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L454
• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L444
• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L298
• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L285
• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L291

Gas Report

• G-01: use unchecked when incrementing the iterator variable
• G-02: use constant state variables wherever you can
• G-03: pack structs
• G-04: use immutable state variables wherever you can
• G-05: remove unnecessary memory caching
• G-06: cancelOrder() can be simplified

G-01: use unchecked when incrementing the iterator variable

Since the iterator can't overflow in most cases because of the conditional check, you can save gas by incrementing it in an unchecked block

• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L415
• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L143
• https://github.com/code-423n4/2022-07-golom/blob/main/contracts/rewards/RewardDistributor.sol#L157
• etc.

G-02: use constant state variables wherever you can

For example, in GolomTrader the WETH variable is never updated. You can set it as a constant to save a lot of gas.

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

G-03: pack structs

The Order struct in GolomTrader can be packed to save gas.

e.g.

struct Order {
    address collection; // NFT contract address
    uint8 v;
    bool isERC721; // standard of the collection , if 721 then true , if 1155 then false
    uint256 tokenId; // order for which tokenId of the collection
    address signer; // maker of order address
    uint256 orderType; // 0 if selling nft for eth , 1 if offering weth for nft,2 if offering weth for collection with special criteria root
    uint256 totalAmt; // price value of the trade // total amt maker is willing to give up per unit of amount
    Payment exchange; // payment agreed by maker of the order to pay on succesful filling of trade this amt is subtracted from totalamt
    Payment prePayment; // another payment , can be used for royalty, facilating trades
    uint256 tokenAmt; // token amt useful if standard is 1155 if >1 means whole order can be filled tokenAmt times
    uint256 refererrAmt; // amt to pay to the address that helps in filling your order
    bytes32 root; // A merkle root derived from each valid tokenId — set to 0 to indicate a collection-level or tokenId-specific order.
    address reservedAddress; // if not address(0) , only this address can fill the order
    uint256 nonce; // nonce of order usefull for cancelling in bulk
    uint256 deadline; // timestamp till order is valid epoch timestamp in secs
    bytes32 r;
    bytes32 s;
}

G-04: use immutable state variables wherever you can

For example, in RewardDistributor, the weth, rewardToken, and startTime variables are only set in the constructor.

https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L68-L69

G-05: remove unnecessary memory caching

In GolomTrader.incrementNonce() you save to memory for no reason. Removing that will save gas:

function incrementNonce() external nonReentrant {ß
    emit NonceIncremented(msg.sender, ++nonces[msg.sender]);
}

G-06: cancelOrder() can be simplified

In GolomTrader.cancelOrder() you call validateOrder() to get the hashed order struct. But, the validateOrder() runs a handful of other operations as well. By calling the _hashOrder() function directly, you can remove all those excess operations and save gas:

function cancelOrder(Order calldata o) public nonReentrant {
    require(o.signer == msg.sender);
    bytes32 hashStruct = _hashOrder(o);
    filled[hashStruct] = o.tokenAmt + 1;
    emit OrderCancelled(hashStruct);
}