Submitted by Trust, also found by zzykxx
https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L819-L829
https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBToken.sol#L202
Similar to other LP pools, In Trader Joe users can call mint() to provide liquidity and receive LP tokens, and burn() to return their LP tokens in exchange for underlying assets. Users collect fees using collectFess(account,binID). Fees are implemented using debt model. The fundamental fee calculation is:
accTokenXPerShare / accTokenYPerShare is an ever increasing amount that is updated when swap fees are paid to the current active bin.
When liquidity is first minted to user, the _accruedDebts is updated to match current _balance * accToken*PerShare. Without this step, user could collect fees for the entire growth of accToken*PerShare from zero to current value. This is done in _updateUserDebts, called by _cacheFees() which is called by _beforeTokenTransfer(), the token transfer hook triggered on mint/burn/transfer.
The critical problem lies in _beforeTokenTransfer:
Note that if _from or _to is the LBPair contract itself, _cacheFees wont be called on _from or _to respectively. This was presumably done because it is not expected that the LBToken address will receive any fees. It is expected that the LBToken will only hold tokens when user sends LP tokens to burn.
This is where the bug manifests - the LBToken address (and 0 address), will collect freshly minted LP tokens fees from 0 to current accToken*PerShare value.
We can exploit this bug to collect the entire reserve assets. The attack flow is:
Note that if the contract did not have the entire collectFees code in an unchecked block, the loss would be limited to the total fees accrued:
If attacker would try to overflow the feesX/feesY totals, the call would revert. Unfortunately, because of the unchecked block feesX/feesY would overflow and therefore there would be no problem for attacker to take the entire reserves.
Attacker can steal the entire reserves of the LBPair.
Paste this test in LBPair.Fees.t.sol:
Manual audit, foundry
Code should not exempt any address from _cacheFees(). Even address(0) is important, because attacker can collectFees for the 0 address to overflow the FeesX/FeesY variables, even though the fees are not retrievable for them.
0x0Louis (Trader Joe) confirmed
Alex the Entreprenerd (judge) commented:
