Trader Joe v2 contest - Tutturu's results

Lines of code

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

Vulnerability details

Impact

The implementation of the _transfer() function in LBToken.sol doesn't check for self-transfers, leading to users being able to mint an unlimited amount of tokens to themselves.

The function caches the balance of the sender (L182) and receiver (L188) before adding and deducing the balances on L181-198. This means that if the sender and receiver are the same address, the address will receive their previous balance + the sent amount, effectively minting tokens to themselves.

Proof of Concept

1. User A has 1000 tokens and self-transfers 1000 tokens
2. Before state is updated the values are cached: _fromBalance = 1000, _toBalance = 1000
3. The balance is deduced from the sender _balances[_id][_from] = _fromBalance (1000) - _amount (1000) => _balances[_id][_from] = 0
4. The balance is added to the receiver (same address) _balances[_id][_from] = _toBalance (1000) + _amount (1000) => _balances[_id][_to] = 2000
5. The attacker has doubled their tokens and can repeat this action as many times as they want

The below test case can be added to LBToken.t.sol to confirm the exploit.

function testSelfTransfer() 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);
        pair.safeTransferFrom(DEV, DEV, _ids[0], amounts[0]);
        // User has minted amounts[0] tokens to themselves, doubling their balance
        assertEq(pair.balanceOf(DEV, _ids[0]), amounts[0] * 2);
    }

Tools Used

Foundry, manual review

Recommended Mitigation Steps

Add a check for self-transfers if (_from == _to) revert LBToken__SelfTransfer(_from), or don't use cached values on L181-198