Golom contest - sseefried's results

Lines of code

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

Vulnerability details

Impact

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

1. The receiver smart contract does not implement a payable receive function.

2. The receiver smart contract does implement a payable fallback function which uses more than 2300 gas unit.

3. The receiver smart contract implements a payable fallback/receive 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.

Recommended Mitigation Steps

Use call instead of transfer to send ETH. Also, the return value must be checked to see if the call is successful.

Lines of code

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

Vulnerability details

Impact

The transferFrom function is discouraged by OpenZeppelin. Function safeTransferFrom will check that recipients are aware of the ERC721 protocol and thereby prevent tokens from being locked forever in contracts that don't provide functionality to transfer them back out.

Recommended Mitigation Steps

Simply replace transferFrom with safeTransferFrom on line 236.

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ce0068c21ecd97c6ec8fb0db08570f4b43029dde/contracts/token/ERC721/ERC721.sol#L395-L417

Vulnerability details

Impact

While VoteEscrowCore.safeTransferFrom does try to call onERC721Received on the receiver it does not check the for the required "magic bytes" which is IERC721.onERC721received.selector in this case. See OpenZeppelin docs for more information.

It's quite possible that a call to onERC721Received could succeed because the contract had a fallback function implemented, but the contract is not ERC721 compliant.

The impact is that NFT tokens may be sent to non-compliant contracts and lost.

Proof of Concept

Lines 604 - 605 are:

try IERC721Receiver(_to).onERC721Received(msg.sender, _from, _tokenId, _data) returns (bytes4) {} catch (
    bytes memory reason

but they should be:

try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {
    return retval == IERC721Receiver.onERC721Received.selector;
} catch (bytes memory reason)

Recommended Mitigation Steps

Implement safeTransferReturn so that it checks the required magic bytes: IERC721Receiver.onERC721Received.selector.

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/core/GolomTrader.sol#L444-L451 https://github.com/code-423n4/2022-07-golom/blob/e5efa8f9d6dda92a90b8b2c4902320acf0c26816/contracts/rewards/RewardDistributor.sol#L298-L305

Vulnerability details

Impact

The Golom project wisely uses time-locks for changing sensitive values like the distributor and the vote escrow contract. However, it has a small flaw. It is possible, once the address is not address(0) to set the address to address(0). Users may be fooled into thinking this is completely benign.

However, once the time delay of one day has passed, setDistributor/addVoteEscrow can be called with a malicious contract address.

Proof of Concept

The following is for setDistributor but a similar PoC applies to addVoteEscrow

• Let evilDistributor be a malicious distributor contract
• Call setDistributor(address(0))
• After 1 days call executeSetDistributor()
• Call setDistributor(evilDistributor)
• Immediately call executeSetDistributor and watch the mayhem unfold

Recommended Mitigation Steps

Add an extra require in GolomTrader.setDistributor as follows (and make the analogous change to RewardDistributor.addVoteEscrow):

function setDistributor(address _distributor) external onlyOwner {
    if (address(distributor) == address(0)) {
        distributor = Distributor(_distributor);
    } else {
        require(_distributor != address(0));
        pendingDistributor = _distributor;
        distributorEnableDate = block.timestamp + 1 days;
    }
}

QA Report

Remarks/Recommendations

• Documentation at docs.golom.io is incomplete in many sections. This made it harder to audit the code that necessary and users will surely want more thorough documentation.

Coding style Improvements

• RewardDistributor.sol:57 can be remove and you can use the literal value 1 day. It is guaranteed to be 86400 seconds

• Magic numbers abound. Here, here, here

• Why not use enums for order status? e.g. Here, here

• Repeated calculations are error prone. What if you change on and forget to change the others? e.g. The percentage calculate of (o.totalAmt * 50) / 10000 here and here and in other places.

• The Timelock contract is saved in a file called Timlock.sol

Documentation improvements

1. GolomTrader has the following comments in payEther but it's a general purpose function used for all sorts of things.

// if royalty has to be paid
payable(payAddress).transfer(payAmt); // royalty transfer to royaltyaddress

2. Perhaps document return values for validateOrder as follows:

function validateOrder(Order calldata o)
    public
    view
    returns (
        uint256 /* order status */,
        bytes32 /* hashed order */,
        uint256 /* amount remaining to be filled */
    )

This avoids having to give them real variable names but makes it really clear what is going on.

3. Change totalAmt to totalAmtPerUnit in struct Order. The name is confusing as it is.

4. The comment // require eth amt is sufficient appears in both fillBid and fillCriteriaBid even though these functions don't accept ETH (and also not marked as payable). Remove this comment.

5. Lines 1108-1109 in VoteEscrowCore seems to be relics of the original Vyper implementation.

// Copying and pasting totalSupply code because Vyper cannot pass by
// reference yet

Test improvements

1. Add this to end of RewardDistributor.specs.ts's test it('should revert if the date is wrong'...

const block = await ethers.provider.getBlock(await ethers.provider.getBlockNumber());
// wait one day
await ethers.provider.send("evm_mine", [block.timestamp + 86400]);
await expect(golomToken.executeSetMinter()).not.to.be.reverted;

2. The title of GolomTrader.specs.ts's test it('should revert if order status is not equal to 3' seems wrong. Is this meant to test whether filling an order twice fails?

3. At the bottom of GolomTrader.specs.ts's test it("should update the filled mapping"... we have

// TODO: fix hash struct
// await expect(golomTrader.filled()).to.be.revertedWith('invalid orderType');

You're not actually testing anything with this test at the moment.

4. No tests for GolomTrader's cancelOrder, fillCriteriaBid, incrementNonce and executeSetDistributor (and stub for fillCriteriaBid is misspelled as fillCriticalBid)

5. No tests in VoterEscrowDelegation.specs.ts at all

L-01: check-effects-interaction pattern violated in fillAsk

The check-effects-interaction pattern should be followed even when there is re-entrancy protection on the function as it is often the case that not all functions have re-entrancy protection.

The effect Line 269 should be moved to before the interaction, the payEther call on line 242.

L-02: Extra ETH can get locked in GolemTrader

This is because check on line 217 is

require(msg.value >= o.totalAmt * amount + p.paymentAmt, 'mgmtm');

But only an amount equal to o.totalAmt * amount + p.paymentAmt is ever paid out.

L-03: Maker of order can create uncancellable order with o.tokenAmt == type(uint256).max

A maker can create an uncancellable order by setting o.tokenAmt to type(uint256).max. This might sound unlikely but it is possible to create a collection that produces 2**256 - 1 combinations pretty easily.

However, an order made with this collection cannot be cancelled because line 315 causes an overflow.

This means that the only way they can cancel the order is to use incrementNonce but this forces them to recreate and sign all other orders they have with that nonce.

L-04: Division by epochTotalFee[epochs[index]] could be division by zero in RewardsDistributor

Although extremely unlikely, no trading could happen for 24 hours leading to, for a given epoch, epochTotalFee[epoch] == 0. This will lead to reversions in functions traderClaim and exchangeClaim.

The impact is not high since there would have been no reward for that epoch but it might be a good idea to add an if statement around the calculation e.g.

if (epochTotalFee[epochs[index]] > 0) {
  reward =
      reward +
      (rewardExchange[epochs[index]] * feesExchange[addr][epochs[index]]) /
      epochTotalFee[epochs[index]];
}

NC-01: Dead code in GolomTrader.validateOrder

The code immediately following the require on line 177 is dead code as it can't ever evaluate to true.

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