Golom contest - chatch's results

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

Non-critical: EIP712 signatures on GolomTrader could be replayed in case of blockchain forks The chainId is burnt into EIP712_DOMAIN_TYPEHASH rather than checked each time.

This means that signatures could be replayed on a forked chain.

See how OpenZeppelin handles this case with _domainSeparatorV4 which is actually being used by the GolomToken.sol via ERC20Votes.

Non-critical: transfer not recommened (SWC-134)

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

transfer is not recommended du to possibility of gas prices changing in future.

Non-critical: hardhat/console.sol import

Removing this will save on deployment gas.

Consider using the hardhat plugin https://www.npmjs.com/package/hardhat-log-remover which helps to strip these out.

Non-critical: GolomTrader distributor could be set to an EOA

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

Consider checking that _distributor has bytecode by using OpenZeppelin's Address.isContract

Non-critical: GomolTrader.validateOrder return variables clarity

Consider using @return natspec comment for each of the return varilables.

Also added a name for each under returns instead of just the types there will increase code readability.

Non-critical: GolomToken uses a floating pragma version

Fixing the version at 0.8.11 will ensure that the testing AND deployment are both done with 0.8.11. It will also be consistent with most of the other contracts in the repository.

Non-critical: GolomToken does not implement ERC165 supportsInterface

Implementing supportsInterface will improve user experience as wallets, block explorers, indexers etc. can see and show the contract implements ERC20 and ERC2612 (and ERC165 itself).

Non-critical: EIP712 signatures on GolomTrader could be replayed in case of blockchain forks

The chainId is burnt into EIP712_DOMAIN_TYPEHASH rather than checked each time.

This means that signatures could be replayed on a forked chain.

See how OpenZeppelin handles this case with _domainSeparatorV4 which is actually being used by the GolomToken.sol via ERC20Votes.

Non-critical: GolomToken event logging

No events are emitted for the following significant actions in GolomToken:

mintAirdrop mintGenesisReward setMinter executeSetMinter

Non-critical: SPDX licensing improvements

1. TokenUriHelper.sol should use a welformed SPDX license identifier comment but instead has:

/// [MIT License]

see https://docs.soliditylang.org/en/latest/layout-of-source-files.html?highlight=spdx#spdx-license-identifier

2. Although the contracts GovernerBravo.sol (sic: GovernerAlpha) and Timlock.sol (sic: Timelock) are out of scope I think it's worth mentioning they have changed the license from the original BSD-3-Clause license in Compound to an MIT license however the BSD license should be retained.

Additionally BSD requires the Copyright notice should be retained. In the case of Compound this is Copyright 2020 Compound Labs, Inc. and it's defined in the LICENSE file in the root of the repository: https://github.com/compound-finance/compound-protocol/blob/master/LICENSE

3. Finally consider making the MIT license clear at the repository level by including a LICENSE file and using the license property in package.json.