Trader Joe v2 contest - __141345__'s results

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBToken.sol#L176-L196

Vulnerability details

Impact

In _transfer(), if a user do a self transfer, the _balances will double, and steal fund from the pair. The pair could be drained.

Proof of Concept

If _from == _to, the self transfer will double the _balances.

// src/LBToken.sol
    function _transfer(
        address _from,
        address _to,
        uint256 _id,
        uint256 _amount
    ) internal virtual {
        uint256 _fromBalance = _balances[_id][_from];
        // ...
        uint256 _toBalance = _balances[_id][_to];

        unchecked {
            _balances[_id][_from] = _fromBalance - _amount;
            _balances[_id][_to] = _toBalance + _amount;
        }

        // ...
    }

Tools Used

Manual analysis.

Recommended Mitigation Steps

In _transfer() add check:

    if (_from == _to) revert();

Variable re-arrangement by storage packing

In src/LBFactory.sol, bool public override creationUnlocked can be placed next to address public override feeRecipient, as a result, 1 slot storage can be saved. According to the currently layout, they both occupy 1 slot, but after re-arrangement, they can be packed into 1 slot.

// src/LBFactory.sol
34-39:
    address public override feeRecipient;

    uint256 public override flashLoanFee;

    bool public override creationUnlocked;

Reference: Layout of State Variables in Storage.

Using bool for storage incurs overhead

// Booleans are more expensive than uint256 or any type that takes up a full

// word because each write operation emits an extra SLOAD to first read the
// slot's contents, replace the bits taken up by the boolean, and then write
// back. This is the compiler's defense against contract upgrades and
// pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

Instances number of this issue: 1

// src/LBFactory.sol
39:     bool public override creationUnlocked;

Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

// src/LBPair.sol
68-70:
    mapping(address => bytes32) private _unclaimedFees;
    mapping(address => mapping(uint256 => Debts)) private _accruedDebts;

// src/LBToken.sol
17-18:
    /// @dev Mapping from token id to account balances
    mapping(uint256 => mapping(address => uint256)) private _balances;
23-24:
    /// @dev Mapping from token id to total supplies
    mapping(uint256 => uint256) private _totalSupplies;

20-21:
    /// @dev Mapping from account to spender approvals
    mapping(address => mapping(address => bool)) private _spenderApprovals;
26-27:
    /// @dev  Mapping from account to set of ids, where user currently have a non-zero balance
    mapping(address => EnumerableSet.UintSet) private _userIds;

Update value order can be adjusted to simplify the code and save gas

For example, to update the num variable with newVal, the current way is as following:

    uint oldVal = num;
    num = newVal;
    emit update(oldVal, newVal);

If the execution order is adjusted, some operations can be saved (memory space allocation, variable assignment), reducing both the deployment and run time gas cost.

    emit update(num, newVal);
    num = newVal;

The operation saved: 1 sload, 1 memory allocation, 1 mstore.

The demo of the gas comparison can be seen here.

There are multiple places can use this trick for optimization, since the updates of parameters are widely and frequently used, the optimization can be beneficial.

Instances number of this issue: 4

// src/libraries/PendingOwnable.sol
    function _transferOwnership(address _newOwner) internal virtual {
        address _oldOwner = _owner;
        _owner = _newOwner;
        _pendingOwner = address(0);
        emit OwnershipTransferred(_oldOwner, _newOwner);
    }

// src/LBFactory.sol
    function setLBPairImplementation(address _LBPairImplementation) external override onlyOwner {
        // ...

        address _oldLBPairImplementation = LBPairImplementation;
        if (_oldLBPairImplementation == _LBPairImplementation)
            revert LBFactory__SameImplementation(_LBPairImplementation);

        LBPairImplementation = _LBPairImplementation;

        emit LBPairImplementationSet(_oldLBPairImplementation, _LBPairImplementation);
    }

    function setFlashLoanFee(uint256 _flashLoanFee) external override onlyOwner {
        uint256 _oldFlashLoanFee = flashLoanFee;

        if (_oldFlashLoanFee == _flashLoanFee) revert LBFactory__SameFlashLoanFee(_flashLoanFee);

        flashLoanFee = _flashLoanFee;
        emit FlashLoanFeeSet(_oldFlashLoanFee, _flashLoanFee);
    }

    function _setFeeRecipient(address _feeRecipient) internal {
        // ...

        address _oldFeeRecipient = feeRecipient;
        if (_oldFeeRecipient == _feeRecipient) revert LBFactory__SameFeeRecipient(_feeRecipient);

        feeRecipient = _feeRecipient;
        emit FeeRecipientSet(_oldFeeRecipient, _feeRecipient);
    }

The above can be changed to:

// src/libraries/PendingOwnable.sol
    function _transferOwnership(address _newOwner) internal virtual {
        emit OwnershipTransferred(_owner, _newOwner);
        _owner = _newOwner;
        _pendingOwner = address(0);
    }

// src/LBFactory.sol
    function setLBPairImplementation(address _LBPairImplementation) external override onlyOwner {
        // ...

        emit LBPairImplementationSet(LBPairImplementation, _LBPairImplementation);

        if (LBPairImplementation == _LBPairImplementation)
            revert LBFactory__SameImplementation(_LBPairImplementation);

        LBPairImplementation = _LBPairImplementation;
    }

    function setFlashLoanFee(uint256 _flashLoanFee) external override onlyOwner {
        emit FlashLoanFeeSet(flashLoanFee, _flashLoanFee);

        if (flashLoanFee == _flashLoanFee) revert LBFactory__SameFlashLoanFee(_flashLoanFee);

        flashLoanFee = _flashLoanFee;
    }

    function _setFeeRecipient(address _feeRecipient) internal {
        // ...

        emit FeeRecipientSet(feeRecipient, _feeRecipient);

        if (feeRecipient == _feeRecipient) revert LBFactory__SameFeeRecipient(_feeRecipient);

        feeRecipient = _feeRecipient;
    }

Variables referred multiple times can be cached in local memory

Even for a memory struct variable, the member variable access still has overhead. It can be saved further by caching the final result.

_pair.activeId can be cached, it is referred 10 times in function swap().

// src/LBPair.sol
    function swap(bool _swapForY, address _to)

Duplicated checks could be refactored to a modifier or function

Instances number of this issue: 12

1. address(0) check

// src/LBFactory.sol
511:        if (_feeRecipient == address(0)) revert LBFactory__AddressZero();

// src/LBRouter.sol
627:        if (address(_token) == address(0)) {

// src/LBToken.sol
207:        if (_account == address(0)) revert LBToken__MintToAddress0();
232:        if (_account == address(0)) revert LBToken__BurnFromAddress0();

// src/libraries/PendingOwnable.sol
60:         if (pendingOwner_ == address(0)) revert PendingOwnable__AddressZero();

2. dual address(0) check

// src/LBPair.sol
111:        if (address(_tokenX) == address(0) || address(_tokenY) == address(0)) revert LBPair__AddressZero();

// src/LBToken.sol
38:         if (_from == address(0) || _to == address(0)) revert LBToken__TransferFromOrToAddress0();

3. _route.length

// src/LBQuoter.sol
59-61:
        if (_route.length < 2) {
            revert LBQuoter_InvalidLength();
        }
139-141:
        if (_route.length < 2) {
            revert LBQuoter_InvalidLength();
        }

4. _LBPair.tokenX check:

// src/LBRouter.sol
212-217:
        ILBPair _LBPair = _getLBPairInformation(
            _liquidityParameters.tokenX,
            _liquidityParameters.tokenY,
            _liquidityParameters.binStep
        );
        if (_liquidityParameters.tokenX != _LBPair.tokenX()) revert LBRouter__WrongTokenOrder();
236-241:
        ILBPair _LBPair = _getLBPairInformation(
            _liquidityParameters.tokenX,
            _liquidityParameters.tokenY,
            _liquidityParameters.binStep
        );
        if (_liquidityParameters.tokenX != _LBPair.tokenX()) revert LBRouter__WrongTokenOrder();
285-289:
        ILBPair _LBPair = _getLBPairInformation(_tokenX, _tokenY, _binStep);
        if (_tokenX != _LBPair.tokenX()) {
            (_tokenX, _tokenY) = (_tokenY, _tokenX);
            (_amountXMin, _amountYMin) = (_amountYMin, _amountXMin);
        }

require()/revert() statements that check input arguments should be at the top of the function

Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas) in a function that may ultimately revert in the unhappy case.

Instances number of this issue:

// src/LBFactory.sol
249:        if (_tokenX == _tokenY) revert LBFactory__IdenticalAddresses(_tokenX);

Duplicate definition of sortTokens()

// src/LBFactory.sol
    function _sortTokens(IERC20 _tokenA, IERC20 _tokenB) private pure returns (IERC20, IERC20) {
        if (_tokenA > _tokenB) (_tokenA, _tokenB) = (_tokenB, _tokenA);
        return (_tokenA, _tokenB);
    }

// src/libraries/JoeLibrary.sol
    function sortTokens(address tokenA, address tokenB) internal pure returns (address token0, address token1) {
        if (tokenA == tokenB) revert JoeLibrary__IdenticalAddresses();
        (token0, token1) = tokenA < tokenB ? (tokenA, tokenB) : (tokenB, tokenA);
        if (token0 == address(0)) revert JoeLibrary__AddressZero();
    }

Suggestion: Just keep one of the definition in the library. In the JoeLibrary one, the check for tokenA == tokenB seems unnecessary.

Unnecessary variables

Some variables are not necessary, can be skipped. Operations saved: memory allocation, mstore.

// src/libraries/TokenHelper.sol
64-68:
        uint256 _internalBalance;
        unchecked {
            _internalBalance = reserve + fees;
        }
        return token.balanceOf(address(this)) - _internalBalance;

numerator and denominator

// src/libraries/JoeLibrary.sol
38-40:
        uint256 numerator = amountInWithFee * reserveOut;
        uint256 denominator = reserveIn * 1000 + amountInWithFee;
        amountOut = numerator / denominator;

51-53:
        uint256 numerator = reserveIn * amountOut * 1000;
        uint256 denominator = (reserveOut - amountOut) * 997;
        amountIn = numerator / denominator + 1;

internal functions only called once can be inlined

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

Instances number of this issue: 14

// src/libraries/Samples.sol
    function pack(
        uint256 _cumulativeBinCrossed,
        uint256 _cumulativeVolatilityAccumulated,
        uint256 _cumulativeId,
        uint256 _timestamp,
        uint256 _initialized
    ) internal pure returns (bytes32 packedSample) {

// src/libraries/BinHelper.sol
    function getIdFromPrice(uint256 _price, uint256 _binStep) internal pure returns (uint24) {

// src/libraries/BitMath.sol
    function closestBitRight(uint256 x, uint8 bit) internal pure returns (uint256 id) {
    function closestBitLeft(uint256 x, uint8 bit) internal pure returns (uint256 id) {

// src/libraries/Buffer.sol
    function before(uint256 x, uint256 n) internal pure returns (uint256 result) {

// src/libraries/FeeHelper.sol
    function getBaseFee(FeeParameters memory _fp) internal pure returns (uint256) {
    function getVariableFee(FeeParameters memory _fp) internal pure returns (uint256 variableFee) {
    function getFeeAmountFrom(FeeParameters memory _fp, uint256 _amountPlusFee) internal pure returns (uint256) {

// src/libraries/Oracle.sol
    function getSampleAt(
        bytes32[65_535] storage _oracle,
        uint256 _activeSize,
        uint256 _activeId,
        uint256 _lookUpTimestamp
    )
    function initialize(bytes32[65_535] storage _oracle, uint256 _index) internal {

// src/libraries/ReentrancyGuardUpgradeable.sol
    function __ReentrancyGuard_init() internal {
    function __ReentrancyGuard_init_unchained() internal {

// src/libraries/TreeMath.sol
    function addToTree(mapping(uint256 => uint256)[3] storage _tree, uint256 _id) internal {
    function removeFromTree(mapping(uint256 => uint256)[3] storage _tree, uint256 _id) internal {

unused functions

// src/libraries/Samples.sol
    function initialized(bytes32 _packedSample) internal pure returns (uint256) {