Maverick contest - RaymondFam's results

Sort _tokenA and _takenB on create()

In create() of Factory.sol, instead of reverting the function on the first require check if the tokens have not been sorted, consider having the function logic refactored as follows to better facilitate the function call:

File: Factory.sol#L58-L83

59: - require(_tokenA < _tokenB, "Factory:TOKENS_MUST_BE_SORTED_BY_ADDRESS");
    + (IERC20 tokenA, IERC20 tokenB) = _tokenA < _tokenB ? (_tokenA, _tokenB) : (_tokenB, _tokenA);
...
72: - _tokenA
73: - _tokenB
    + tokenA
    + tokenB
...
80: - emit PoolCreated(address(pool), _fee, _tickSpacing, _activeTick, _lookback, protocolFeeRatio, _tokenA, _tokenB);
    + emit PoolCreated(address(pool), _fee, _tickSpacing, _activeTick, _lookback, protocolFeeRatio, tokenA, tokenB);

Lines too long

Lines in source code are typically limited to 80 characters, but it’s reasonable to stretch beyond this limit when need be as monitor screens theses days are comparatively larger. Considering the files will most likely reside in GitHub that will have a scroll bar automatically kick in when the length is over 164 characters, all code lines and comments should be split when/before hitting this length. Keep line width to max 120 characters for better readability where possible.

Here are some of the instances entailed:

File: Factory.sol#L58 File: Pool.sol#L238 File: Pool.sol#L260 File: Pool.sol#L270 File: Pool.sol#L294 File: Pool.sol#L516 File: Pool.sol#L534 File: Pool.sol#L597 File: Pool.sol#L602 File: Pool.sol#L639 File: PoolInspector.sol#L33 File: PoolInspector.sol#L107

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

Here are some of the contract instances partially lacking NatSpec:

File: Factory.sol File: Router.sol

Here are some of the contract instances lacking NatSpec in its entirety:

File: Deployer.sol File: Pool.sol File: PoolInspector.sol File: Position.sol

Modularity on import usages

For cleaner Solidity code in conjunction with the rule of modularity and modular programming, import only what you need with curly braces instead of adopting the global import approach.

For instance, the import instances below could be refactored as follows:

File: PoolInspector.sol#L4-L9

import {Pool} from "./Pool.sol";
import {ISwapCallback} from t "../interfaces/ISwapCallback.sol";
import {BinMath, PRBMathUD60x18} from"../libraries/Bin.sol";
import {Math} from "../libraries/Math.sol";
import {Constants} from "../libraries/Constants.sol";
import {IPool} from "../interfaces/IPool.sol";

Two-step transfer of ownership

When setting a new owner, it is entirely possible to accidentally transfer ownership to an uncontrolled and/or non-valid account, breaking all function calls requiring msg.sender == owner. Consider implementing a two step process where the current owner nominates an account that would need to call acceptOwnership() for the transfer of ownership to fully succeed. This will also ensure the new owner is going to be fully aware of the ownership assigned/transferred.

Here is the instance entailed:

File: Factory.sol#L40-L44

    function setOwner(address _owner) external {
        require(msg.sender == owner && _owner != address(0), "Factory:NOT_ALLOWED");
        owner = _owner;
        emit SetFactoryOwner(_owner);
    }

Events associated with setter functions

Consider having events associated with setter functions emit both the new and old values instead of just the new value.

Here are some of the instances entailed:

File: Factory.sol

37:        emit SetFactoryProtocolFeeRatio(_protocolFeeRatio);

43:        emit SetFactoryOwner(_owner);

File: Position.sol#L25

        emit SetMetadata(metadata);

Sanity checks at the constructor

Adequate zero address and zero value checks should be implemented at the constructor to avoid accidental error(s) that could result in non-functional calls associated with it particularly when assigning immutable variables.

Here are some of the instances entailed:

File: Pool.sol#L73-L82

        fee = _fee;
        state.protocolFeeRatio = _protocolFeeRatio;
        tickSpacing = _tickSpacing;
        state.activeTick = _activeTick;
        twa.lookback = _lookback;
        tokenA = _tokenA;
        tokenB = _tokenB;
        position = _position;
        tokenAScale = _tokenAScale;
        tokenBScale = _tokenBScale;

Descriptive and Verbose Options

Consider making the error messages more verbose and distinct so that all other peer developers would better be able to comprehend the intended statement logic, significantly enhancing the code readability.

Here are some of the instances entailed:

File: Pool.sol

87:        require(msg.sender == position.ownerOf(tokenId) || msg.sender == position.getApproved(tokenId), "P");

93:        require((currentState.status & LOCKED == 0) && (allowInEmergency || (currentState.status & EMERGENCY == 0)), "E");

168:        require(previousABalance + tokenAAmount <= _tokenABalance() && previousBBalance + tokenBAmount <= _tokenBBalance(), "A");

303:        require(previousBalance + amountIn <= (tokenAIn ? _tokenABalance() : _tokenBBalance()), "S");

Unneeded return keyword on named returns

Functions adopting named returns need not use the return keyword when returning their corresponding values.

For instance, the two return instances below may be refactored as follows:

File: Pool.sol#L312-L318

    function getBin(uint128 binId) external view override returns (BinState memory bin) {
-        return bins[binId].state;
+        bins[binId].state;
    }

    function balanceOf(uint256 tokenId, uint128 binId) external view override returns (uint256 lpToken) {
-        return bins[binId].balances[tokenId];
+        bins[binId].balances[tokenId];
    }

Use delete to clear variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address or a boolean to false. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic.

For instance, the a = 0 instance below may be refactored as follows when _removeBin() is internally invoked:

File: Pool.sol#L359

-        binPositions[tick][kind] = 0;
+        delete binPositions[tick][kind];

Similarly, the a = false instance below may be refactored as follows after _removeBin() is internally invoked:

File: Pool.sol#L235

-                isActive = false;
+                delete isActive;

Emission of events at the constructor

State variable assignment that entails an event emission should also be inclusive at the construct to be consistent with the intended design.

For instance, the constructor instance below may be refactored as follows:

File: Position.sol#L19

-        metadata = _metadata;
+        setMetadata( _metadata);

Missing checks for contract existence

Performing low-level calls without confirming contract’s existence (not yet deployed or have been destructed) could return success even though no function call was executed as documented in the link below:

https://docs.soliditylang.org/en/v0.8.7/control-structures.html#error-handling-assert-require-revert-and-exceptions

Consider having account existence checked prior to calling if needed.

Note: A report associated with Pool.sol and SafeERC20Min.sol delineating the malicious attack has been separately submitted.

Here are the other instances entailed that may not have posed a risk but for informational purpose:

File: TransferHelper.sol

14:        (bool success, bytes memory data) = token.call(abi.encodeWithSelector(IERC20.transferFrom.selector, from, to, value));

24:        (bool success, bytes memory data) = token.call(abi.encodeWithSelector(IERC20.transfer.selector, to, value));

34:        (bool success, bytes memory data) = token.call(abi.encodeWithSelector(IERC20.approve.selector, to, value));

43:        (bool success, ) = to.call{value: value}(new bytes(0));

Use enum for lock and action states

Consider adopting enum(s) on the following sets of constants to make the code more presentable and structured:

File: Pool.sol#L31-L38

    uint8 constant NO_EMERGENCY_UNLOCKED = 0;
    uint8 constant LOCKED = 1;
    uint8 constant EMERGENCY = 2;

    uint256 constant ACTION_EMERGENCY_MODE = 911;
    uint256 constant ACTION_SET_PROTOCOL_FEES = 1;
    uint256 constant ACTION_CLAIM_PROTOCOL_FEES_A = 2;
    uint256 constant ACTION_CLAIM_PROTOCOL_FEES_B = 3;

Rebasing tokens

The protocol does not mention whether or not rebasing tokens will be supported. But, if they will be, traders can steal output token for which swapCallback() can be invoked by expanding the total supply of the rebasing token.

File: Pool.sol#L302

        ISwapCallback(msg.sender).swapCallback(amountIn, amountOut, data);

Apparently, the contract associated with the external call above does not have to be a shared router and can be separately implemented by the trader. For a created pool, it is possible that the input token is rebasing, allowing anyone to trigger the rebasing event. When calling swap(), swapCallback() in the callee contract implemented by the trader can be executed to call another contract, which has the admin privilege for the rebasing token to trigger the rebasing event. When the rebasing event expands the input token's total supply, calling swapCallback() does not need to send additionally enough input token to the pool, and the pool's balance of the input rebasing token can still become larger enough than its previous input token balance. For this reason, calling swap() will not revert in this situation, and the trader can receive the correspondingly extra output token amount although the trader has not sent in enough input tokens to the pool.

Split require statements using &&

Instead of using the && operator in a single require statement to check multiple conditions, using multiple require statements with 1 condition per require statement will save 3 GAS per &&.

Here are some of the instances entailed:

File: Factory.sol

41:        require(msg.sender == owner && _owner != address(0), "Factory:NOT_ALLOWED");

60:        require(_fee > 0 && _fee < 1e18, "Factory:FEE_OUT_OF_BOUNDS");

62:        require(_lookback >= 3600 && _lookback <= uint16(type(int16).max), "Factory:LOOKBACK_OUT_OF_BOUNDS");

Unused imports

Importing source units that are not referenced in the contract increases the size of deployment. Consider removing them to save some gas. If there was a plan to use them in the future, a comment explaining why they were imported would be helpful.

Here are some of the instances entailed:

File: PoolInspector.sol#L7-L8

import "../libraries/Math.sol";
import "../libraries/Constants.sol";

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =).

As an example, consider replacing >= with the strict counterpart > in the following inequality instance:

File: FixedPriceFactory.sol#L31

- require(_lookback >= 3600 && _lookback <= uint16(type(int16).max), "Factory:LOOKBACK_OUT_OF_BOUNDS");
+ require(_lookback > 3600 - 1 && _lookback <= uint16(type(int16).max), "Factory:LOOKBACK_OUT_OF_BOUNDS");     

Similarly, consider replacing <= with the strict counterpart < in the following inequality instance, as an example:

File: Factory.sol#L27

- require(_protocolFeeRatio <= ONE_3_DECIMAL_SCALE, "Factory:PROTOCOL_FEE_CANNOT_EXCEED_ONE");
+ require(_protocolFeeRatio < ONE_3_DECIMAL_SCALE + 1, "Factory:PROTOCOL_FEE_CANNOT_EXCEED_ONE");

Use storage instead of memory for structs/arrays

A storage pointer is cheaper since copying a state struct in memory would incur as many SLOADs and MSTOREs as there are slots. In another words, this causes all fields of the struct/array to be read from storage, incurring a Gcoldsload (2100 gas) for each field of the struct/array, and then further incurring an additional MLOAD rather than a cheap stack read. As such, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables will be much cheaper, involving only Gcoldsload for all associated field reads. Read the whole struct/array into a memory variable only when it is being returned by the function, passed into a function that requires memory, or if the array/struct is being read from another memory array/struct.

Here are some of the instances entailed:

File: Pool.sol

118:        State memory currentState = checkReentrancy(false, true);

125:        AddLiquidityTemp memory temp;

128:            AddLiquidityParams memory param = params[i];

194:            RemoveLiquidityParams memory param = params[i];

225:            RemoveLiquidityParams memory param = params[i];

261:        State memory currentState = checkReentrancy(false, true);

264:        Delta.Instance memory delta;

278:            Delta.Instance memory newDelta;

390:                BinMap.Active memory active = moveData.activeList[i];

452:            MoveData memory moveData;

469:            MoveData memory moveData;

616:        uint128[] memory currentBinIds;

+= and -= cost more gas

+= and -= generally cost 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop.

For instance, the += instances below may be refactored as follows:

File: Pool.sol#L142-L143

- tokenAAmount += temp.deltaA;
- tokenBAmount += temp.deltaB;
+ tokenAAmount = tokenAAmount + temp.deltaA;
+ tokenBAmount = tokenBAmount + temp.deltaB;

|| costs less gas than its equivalent &&

Rule of thumb: (x && y) is (!(!x || !y))

Even with the 10k Optimizer enabled: ||, OR conditions cost less than their equivalent &&, AND conditions.

For instance, the instance below may be refactored as follows:

File: Pool.sol#L233

- if (bin.state.reserveA == 0 && bin.state.reserveB == 0 && isActive) {
+ if (!(bin.state.reserveA != 0 || bin.state.reserveB != 0 || !isActive)) {

Ternary over if ... else

Using ternary operator instead of the if else statement saves gas.

For instance, the code block below may be refactored as follows:

File: Pool.sol#L405-L410

   currentBinId < moveData.firstBinId
       ? {
           moveData.firstBinId = currentBinId;
       }
       : {
           moveData.mergeBins[moveData.mergeBinCounter] = currentBinId;
           moveData.mergeBinCounter++;
       }

Unchecked SafeMath saves gas

"Checked" math, which is default in ^0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive than the current SafeMath, one should keep in mind that these checks will increase the cost of "basic math operation" that were not previously covered. This particularly concerns variable increments in for loops. When no arithmetic overflow/underflow is going to happen, unchecked { ++i ;} to use the previous wrapping behavior further saves gas just as in the for loop below as an example:

File: Pool.sol#L180-L183

-        for (uint256 i; i < binIds.length; i++) {
+        for (uint256 i; i < binIds.length;) {
            Bin.Instance storage bin = bins[binIds[i]];
            bin.migrateBinsUpStack(bins, maxRecursion);

+            unchecked {
+                ++i;
+            }
        }

Use of named returns for local variables saves gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable.

For instance, the code block instance below may be refactored as follows:

File: Pool.sol#L97-L99

-    function getState() external view override returns (State memory) {
-        return state;
+    function getState() external view override returns (State memory _state) {
+        _state = state;

Function Order Affects Gas Consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the Optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
  solidity: {
    version: "0.8.0",
    settings: {
      optimizer: {
        enabled: true,
        runs: 1000,
      },
    },
  },
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Payable Access Control Functions Costs Less Gas

Consider marking functions with access control as payable. This will save 20 gas on each call by their respective permissible callers for not needing to have the compiler check for msg.value.

Here are some of the instances entailed:

File: Position.sol#L23

    function setMetadata(IPositionMetadata _metadata) external onlyOwner {