Trader Joe v2 contest - Lambda's results

Lines of code

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

Vulnerability details

Impact

LBToken._transfer caches the _fromBalance and _toBalance and then updates the _balances mapping based on these cached values. This does not work correctly when _from == _to. In this case, we have (for _from == _to == address(A):

_balances[_id][address(A)] = _fromBalance - _amount;
_balances[_id][address(A)] = _toBalance + _amount;

The first decrement of the balance is therefore simply discarded and the balance is only incremented. An attacker can double his own balance like this.

Proof Of Concept

The following diff shows a scenario where an attacker doubles his own balance by a self-transfer:

--- a/test/LBToken.t.sol
+++ b/test/LBToken.t.sol
@@ -91,6 +91,9 @@ contract LiquidityBinTokenTest is TestHelper {
         assertEq(pair.balanceOf(DEV, _ids[0]), 0);
         assertEq(pair.balanceOf(ALICE, _ids[0]), 0);
         assertEq(pair.balanceOf(BOB, _ids[0]), amountIn / 3);
+        vm.prank(BOB);
+        pair.safeTransferFrom(BOB, BOB, _ids[0], amountIn / 3);
+        assertEq(pair.balanceOf(BOB, _ids[0]), amountIn / 3 * 2);
     }
 
     function testSafeBatchTransferNotApprovedReverts() public {

Recommended Mitigation Steps

Do not allow self transfers.

Lines of code

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

Vulnerability details

Impact

LBRouter.removeLiquidity reorders tokens when the user did not pass them in the pair order (ascending order):

if (_tokenX != _LBPair.tokenX()) {
            (_tokenX, _tokenY) = (_tokenY, _tokenX);
            (_amountXMin, _amountYMin) = (_amountYMin, _amountXMin);
}

However, when returning amountX and amountY, it is ignored if the order was changed:

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

Therefore, when the order of the tokens is swapped by the function, the return value amountX ("Amount of token X returned") in reality is the amount of the user-provided token Y that is returned and vice versa.

Because this is an exposed function that third-party protocols / contracts will use, this can cause them to malfunction. For instance, when integrating with Trader Joe, something natural to do is:

(uint256 amountAReceived, uint256 amountBReceived) = LBRouter.removeLiquidity(address(tokenA), address(tokenB), ...);
contractBalanceA += amountAReceived;
contractBalanceB += amountBReceived;

This snippet will only be correct when the token addresses are passed in the right order, which should not be the case. When they are not passed in the right order, the accounting of third-party contracts will be messed up, leading to vulnerabilities / lost funds there.

Proof Of Concept

First consider the following diff, which shows a scenario when LBRouter does not switch tokenX and tokenY, resulting in correct return values:

--- a/test/LBRouter.Liquidity.t.sol
+++ b/test/LBRouter.Liquidity.t.sol
@@ -57,7 +57,9 @@ contract LiquidityBinRouterTest is TestHelper {
 
         pair.setApprovalForAll(address(router), true);
 
-        router.removeLiquidity(
+        uint256 token6BalBef = token6D.balanceOf(DEV);
+        uint256 token18BalBef = token18D.balanceOf(DEV);
+        (uint256 amountFirstRet, uint256 amountSecondRet) = router.removeLiquidity(
             token6D,
             token18D,
             DEFAULT_BIN_STEP,
@@ -70,7 +72,9 @@ contract LiquidityBinRouterTest is TestHelper {
         );
 
         assertEq(token6D.balanceOf(DEV), amountXIn);
+        assertEq(amountXIn, token6BalBef + amountFirstRet);
         assertEq(token18D.balanceOf(DEV), _amountYIn);
+        assertEq(_amountYIn, token18BalBef + amountSecondRet);
     }
 
     function testRemoveLiquidityReverseOrder() public {

This test passes (as it should). Now, consider the following diff, where LBRouter switches tokenX and tokenY:

--- a/test/LBRouter.Liquidity.t.sol
+++ b/test/LBRouter.Liquidity.t.sol
@@ -57,12 +57,14 @@ contract LiquidityBinRouterTest is TestHelper {
 
         pair.setApprovalForAll(address(router), true);
 
-        router.removeLiquidity(
-            token6D,
+        uint256 token6BalBef = token6D.balanceOf(DEV);
+        uint256 token18BalBef = token18D.balanceOf(DEV);
+        (uint256 amountFirstRet, uint256 amountSecondRet) = router.removeLiquidity(
             token18D,
+            token6D,
             DEFAULT_BIN_STEP,
-            totalXbalance,
             totalYBalance,
+            totalXbalance,
             ids,
             amounts,
             DEV,
@@ -70,7 +72,9 @@ contract LiquidityBinRouterTest is TestHelper {
         );
 
         assertEq(token6D.balanceOf(DEV), amountXIn);
+        assertEq(amountXIn, token6BalBef + amountSecondRet);
         assertEq(token18D.balanceOf(DEV), _amountYIn);
+        assertEq(_amountYIn, token18BalBef + amountFirstRet);
     }
 
     function testRemoveLiquidityReverseOrder() public {

This test should also pass (the order of the tokens was only switched), but it does not because the return values are mixed up.

Recommended Mitigation Steps

Add the following statement in the end:

if (_tokenX != _LBPair.tokenX()) {
	return (amountY, amountX);
}

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/37258d595d596c195507234f795fa34e319b0a68/src/LBToken.sol#L237

Vulnerability details

Impact

In LBToken._burn, the _beforeTokenTransfer hook is called with from = address(0) and to = _account:

_beforeTokenTransfer(address(0), _account, _id, _amount);

Through a lucky coincidence, it turns out that this in the current setup does not cause a high severity issue. _burn is always called with _account = address(this), which means that LBPair._beforeTokenTransfer is a NOP. However, this wrong call is very dangerous for future extensions or protocol that built on top of the protocol / fork it.

Proof Of Concept

Let's say the protocol is extended with some logic that needs to track mints / burns. The canonical way to do this would be:

function _beforeTokenTransfer(
        address _from,
        address _to,
        uint256 _id,
        uint256 _amount
    ) internal override(LBToken) {
	if (_from == address(0)) {
		// Mint Logic
	} else if (_to == address(0)) {
		// Burn Logic
	}
}

Such an extension would break, which could lead to loss of funds or a bricked system.

Recommended Mitigation Steps

Call the hook correctly:

_beforeTokenTransfer(_account, address(0), _id, _amount);