Maverick contest - Mukund's results

INDEX

1. Missing address(0) check

Pool.sol#L320-L330 Pool.sol#L339-L350

2. require() statements should have descriptive reason strings

there is no reason in require statement why it get reverted Pool.sol#L333 Pool.sol#L342 Router.sol#L106 Router.sol#L205-L206

3. File is missing NatSpec

consider adding NatSpec for better understanding of contract Factory.sol Pool.sol PoolInspector.sol Position.sol

4. Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed. IPool.sol#L8-L20

5. Contracts are not using their OZ Upgradeable counterparts

The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour. Factory.sol#L4-L5 Pool.sol#L4 Position.sol#L4-L7 PositionMetadata.sol#L5-L6

6. Critical Changes Should Use Two-step Procedure

The critical procedures should be two step process. Factory.sol#L33-L38 Factory.sol#L40-L44 Pool.sol#L332-L337 Position.sol#L23-L26

7. Missing parameter validation

some value in constructors are not checked for invalid values Pool.sol#L60-L84

8. Usage of transfer can lead to loss of funds

The funds that are to be sent can be lost. The issues with transfer() are outlined here: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/ Router.sol#L91

9. Implementation contract may not be initialized

OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5 Pool.sol#L60-L84

10.USE NAMED IMPORTS INSTEAD OF PLAIN `IMPORT ‘FILE.SOL’

for example use it like import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; Router.sol#L4-L16 Factory.sol#L4-L12 Pool.sol#L4-L20 PoolInspector.sol#L4-L9 Position.sol#L4-L10

1. Using uncheck block for operation that cannot overflow/underflow saves gas

for example i++ in every loop is not likely to overflow so its better to uncheck it to save gas Pool.sol#L127 Pool.sol#L180 Pool.sol#L193 Pool.sol#L224 Pool.sol#L389 Pool.sol#L414 Pool.sol#L523 Pool.sol#L540

2. Use assembly to write address storage values

for example instead of rewardToken = token_; write it like assembly{sstore(rewardToken.slot, token_)}; this

3. Splitting && in require function saves gas

splitting && in require function and writing it separately saves gas Factory.sol#L41 Factory.sol#L60 Factory.sol#L62 Pool.sol#L93 Pool.sol#L168

4. Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier or require such as onlyOwner-admin is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2) which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost. Position.sol#L23-L26) PositionMetadata.sol#L17-L19

5. X += Y cost more gas than X = X + Y for state variable

BinMap.sol#L54 BinMap.sol#L97 Pool.sol#L229 Pool.sol#L230

6. Structs can be packed into fewer storage slots

Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings. Pool.sol#L105-L111 PoolInspector.sol#L14-L21

7. Using uint/int smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size. Factory.sol#L16 Factory.sol#L20 Factory.sol#L22 Factory.sol#L58 Pool.sol#L31-L33 Pool.sol#L40 Pool.sol#L57-L58 PoolInspector.sol#L15-L20

8. Using calldata instead of memory for read-only arguments in external functions saves gas

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one Pool.sol#L516 PoolInspector.sol#L23 PoolInspector.sol#L77