Golom contest - StyxRave's results

Lines of code

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

Vulnerability details

Using payable(address).transfer has been deprecated in favor of using .call{value:...}("") as the proper way of sending ETH. Using transfer or send will make transactions fail when the address corresponds to a contract that does not implement a payable function using less than 2300 gas (e.g. some multisigs or proxied contracts). As gas costs can change over time and smart contracts are increasingly used as investment aggregators / fund managers over EOAs, it's recommended to stop using transfer altogether.

In the specific context of this project, current implementation will prevent such contracts from being able to correctly interact with the core functionality of this protocol, as the payEther function is used for all ETH transfers when filling orders and settling balances.

Domain specific require statements with no messages - functions that comprise the core functionality of the protocol and are intended to be used by a third party should probably give meaningful error messages in their require statements. fillAsk falls more or less in line with this, but fillBid and fillCriteriaBid seem to be missing some additional information.

Variable declarations shadowing inherited variable / function - Functions in VoteEscrowDelegation.sol name variables or input arguments as tokenId and checkpoint. Both names are already being used by a state variable and a function declared in VoteEscrowCore.sol. It has no practical impact as local variables take precedence in each scope, but this should be avoided to prevent confusion when developing (same name+type variables with vastly different semantics as is the case with tokenId may throw anyone off when reading code).

Missing event emission for critical updates and core functionality - traderClaim, exchangeClaim and multiStakerClaim should probably emit events for easier off-chain monitoring. Additionally, modifying critical parameters and addresses of contracts should always emit events signaling each modification (e.g. both preparing and setting the corresponding Distributor or adding vote escrow / changing Trader) for both transparency and auditing concerns.

Copypasted docstring fail - traderRewards mentions a returned tuple which is not there and exchangeRewards describes trader rewards. Should probably revisit documentation of external functions intended for third-party integration / use.

Extract inlined constant maxSupply to an explicit contract constant

• Loops - general improvements for all loops in scope: GolomTrader.sol: 1, VoteEscrowDelegation.sol: 2, 3, 4, RewardDistributor.sol: 5, 6, 7, 8, 9, 10, 11.

  • Cache loop length in stack instead of reading from memory or storage every iteration. Replace for (...; i < arr.length; ...) { ... } with uint cachedLength = arr.length; for (...; i < cachedLength; ...) { ... }.
  • Cache storage variables that are read within the loop outside of it - mainly epoch access in traderClaim, exchangeClaim and multiStakerClaim. e.g. uint currentEpoch = epoch; for ...
  • Cache index dependent memory / storage data read at the beginning of the loop / mainly claimed[i] access in stakerRewards and tokenids[tindex] & epochs[index] in multiStakerClaim.
  • Use ++i for increments if the resulting value is not immediately needed. It costs less gas compared to i++ or i+=1 - as no temporary variable needs to exist in order to return the unincremented value.
  • Use unchecked arithmetic in counters, Solidity 0.8.x checks for overflow/underflow by default, which is not needed in this case. Replace for (...; ... ; ++i) { ... } with for (...; ...; ) { ... unchecked { ++i; } }

• Cache non constant nor immutable storage variables when reading them multiple times - epoch in addFee can be cached to a stack variable uint _epoch = epoch. Same applies to contract addresses such as ve, rewardToken or weth if they are not made immutable or constant.

• Variables that could be made constant / immutable - reducing mutability saves gas on access. Multichain deployment with different addresses for depended on contracts can be handled in the deployment phase instead of the constructor.

  • GolomTrader - WETH address
  • RewardDistributor - startTime -- initialized in the constructor to a constant value
  • RewardDistributor - rewardToken -- initialized in the constructor to a constant value
  • RewardDistributor - weth -- initialized in the constructor to a constant value

• Redudant state var initialization of epoch = 0 - default value on construction is already 0.

• Extract repeated fee calculation result to local variable

• Use less than 32 characters for require messages unless necessary - storage is partitioned in 32 bytes slots, each additional slot needed will increase gas on deployment and require an additional read each time is accessed. 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11.

• Use custom errors over revert strings - 1 and 2.

• Try to move expensive storage access after possible reverts - e.g. in fillCriteriaBid proof verification