Trader Joe v2 contest - RaoulSchaffranek's results

Lines of code

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

Vulnerability details

Impact

Users can "mint" free LPTokens to themselves. They can then redeem these LBTokens to drain assets from the respective LBPair-instance. To do that, a user first has to obtain some LBTokens. He can then transfer the tokens to himself. Due to a bug in the internal accounting logic, his balance will increase after the transfer.

The bug can also be used to break the following contract-invariant, which can then lead to unexpected behaviors in different parts of the contract:

forall id in uint256 . LBToken.totalSupply(id) = sum({LBToken.balanceOf(user, id) |  user in address})

Proof of Concept

Here is a simple exploit that demonstrates the vulnerability. In this example, a malicious user doubles his LBToken balance with a single call to safeTransferFrom (notice that this bug also works with the safeBatchTransferFrom-function).

Let $id be the id of an arbitrary LBToken bin. Let $alice be the address of an attacker. Let $amount be $alice's balance of $id, i.e. _balances[$id][$alice] = $amount. Assume $amount > 0.

Now, $alice executes a token transfer with herself as _from and _to-address: safeTransferFrom($alice, $alice, $id, $amount).

After the transfer, $alice's balance will have doubled, i.e., _balances[$id][$alice] = 2 * $amount.

Hence, $alice could double her balance almost for free (only paying gas costs). See below for the mechanized exploit code.

---

Here is a foundry test that demonstrates the exploit. Notice a passing test means that the exploit was successful.

diff --git a/test/LBToken.t.sol b/test/LBToken.t.sol
index d263153..517721a 100644
--- a/test/LBToken.t.sol
+++ b/test/LBToken.t.sol
@@ -27,6 +27,18 @@ contract LiquidityBinTokenTest is TestHelper {
         pair = createLBPairDefaultFees(token6D, token18D);
     }
 
+    function testSafeTransferExploit() public {
+        uint256 amountIn = 1e18;
+
+        (uint256[] memory _ids, , , ) = addLiquidity(amountIn, ID_ONE, 5, 0);
+
+        uint256 id = _ids[0];
+        uint256 amount = pair.balanceOf(DEV, id);
+
+        pair.safeTransferFrom(DEV, DEV, id, amount);
+        assertEq(pair.balanceOf(DEV, id), 2 * amount);
+    }
+
     function testSafeBatchTransferFrom() public {
         uint256 amountIn = 1e18;
 

Tools Used

None.

Recommended Mitigation Steps

The problem is that the internal _transfer-function does not handle the case correctly when the _from-address equals the _to-address.

Here is what the _transfer-function does wrong that enables the above attack:

1. reads the balances of _from and _to from storage and caches them in local memory.
2. Substract the given amount to the cached balance of the _to-address and store the result in the storage.
3. Add the given amount to the cached balance of the _from-address and store the result in the storage.

Notice when we have _from = _to, step 3 will overwrite the effect of step 2. Hence, we only increment the receiver's balance but do not decrease the original owner's balance. Since both addresses are the same, we only increment the user's balance.

• Don't rely on cached copies of the user's balance when updating it.
• Don't update the balances if _from = _to.