Trader Joe v2 contest - 0x1f8b's results

• Low
  • 1. Outdated compiler
  • 2. Lack of checks supportsInterface
  • 3. Integer overflow by unsafe casting
  • 4. Avoid the possibility of an evil factory
• Non critical
  • 5. Avoid hardcoded values

Low

1. Outdated compiler

The pragma version used are:

pragma solidity ^0.8.0;

The minimum required version must be 0.8.17; otherwise, contracts will be affected by the following important bug fixes:

0.8.3:

• Optimizer: Fix bug on incorrect caching of Keccak-256 hashes.

0.8.4:

• ABI Decoder V2: For two-dimensional arrays and specially crafted data in memory, the result of abi.decode can depend on data elsewhere in memory. Calldata decoding is not affected.

0.8.9:

• Immutables: Properly perform sign extension on signed immutables.
• User Defined Value Type: Fix storage layout of user defined value types for underlying types shorter than 32 bytes.

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

0.8.17

• Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.

Apart from these, there are several minor bug fixes and improvements.

2. Lack of checks supportsInterface

The EIP-165 standard helps detect that a smart contract implements the expected logic, prevents human error when configuring smart contract bindings, so it is recommended to check that the received argument is a contract and supports the expected interface.

Reference:

• https://eips.ethereum.org/EIPS/eip-165

Affected source code:

• LBQuoter.sol:45-47

3. Integer overflow by unsafe casting

Keep in mind that the version of solidity used, despite being greater than 0.8, does not prevent integer overflows during casting, it only does so in mathematical operations.

It is necessary to safely convert between the different numeric types.

Recommendation:

Use a safeCast from Open Zeppelin.

        checkpoint.votes = uint192(_newTotalVotes);

Affected source code:

• LBPair.sol:393
• LBPair.sol:670
• LBPair.sol:671
• LBRouter.sol:111
• LBRouter.sol:134
• LBRouter.sol:164
• LBRouter.sol:165
• LBRouter.sol:179
• FeeHelper.sol:59
• FeeHelper.sol:85
• FeeHelper.sol:150

4. Avoid the possibility of an evil factory

The method removeLiquidity in LBRouter allows to change the tokens in an insecure way if they are out of order, which allows a factory that returns a pair of totally different tokens for two given tokens, to eliminate liquidity despite using different tokens.

    function removeLiquidity(
        IERC20 _tokenX,
        IERC20 _tokenY,
        uint16 _binStep,
        uint256 _amountXMin,
        uint256 _amountYMin,
        uint256[] memory _ids,
        uint256[] memory _amounts,
        address _to,
        uint256 _deadline
    ) external override ensure(_deadline) returns (uint256 amountX, uint256 amountY) {
        ILBPair _LBPair = _getLBPairInformation(_tokenX, _tokenY, _binStep);
        if (_tokenX != _LBPair.tokenX()) {
+           if (_tokenX != _LBPair.tokenY() || _tokenY != _LBPair.tokenX()) revert();
            (_tokenX, _tokenY) = (_tokenY, _tokenX);
            (_amountXMin, _amountYMin) = (_amountYMin, _amountXMin);
        }

        (amountX, amountY) = _removeLiquidity(_LBPair, _amountXMin, _amountYMin, _ids, _amounts, _to);
    }

Affected source code:

• LBRouter.sol:286-289

---

Non critical

5. Avoid hardcoded values

It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these values from the source code.

Affected source code:

• LBQuoter.sol:85-89
• LBQuoter.sol:163-166
• JoeLibrary.sol:37-39
• JoeLibrary.sol:51-52