Trader Joe v2 contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

LBToken._transfer() won't work properly when _from == _to.

Users can double their balances as they want by transferring the tokens to their accounts again.

As a result, the token will be useless.

Proof of Concept

Inside the _transfer(), it uses _fromBalance and _toBalance to update the balances after the transfer.

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

But it won't work properly when _from == to so users can increase their balance as they like.

I've created a test for the proof and you can copy the below code here and check the result.

    vm.prank(BOB);
    pair.setApprovalForAll(DEV, true);
    pair.safeTransferFrom(BOB, BOB, _ids[0], amounts[0]);
    assertEq(pair.balanceOf(BOB, _ids[0]), 2 * amounts[0]);

We can see the balance is doubled when BOB transfers the entire tokens to BOB again.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

We can modify this part like below.

_balances[_id][_from] -= _amount;
_balances[_id][_to] += _amount;

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBRouter.sol#L725

Vulnerability details

Impact

It calculates the amountsIn wrongly here and the function returns the wrong result.

Proof of Concept

Currently, _getAmountsIn() calculates the amountsIn like below.

amountsIn[i - 1] = (_reserveIn * amountOut_ * 1_000) / (_reserveOut - amountOut_ * 997) + 1;

As we can see here, we should fix (_reserveOut - amountOut_ * 997) to ((_reserveOut - amountOut_) * 997).

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

This line should be fixed like below.

amountsIn[i - 1] = (_reserveIn * amountOut_ * 1_000) / ((_reserveOut - amountOut_) * 997) + 1;

Judge has assessed an item in Issue #446 as M risk. The relevant finding follows:

[L-01] There should be an upper limit for LBFactory.flashLoanFee https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBFactory.sol#L66 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBFactory.sol#L474 If the admin sets the flashLoanFee too high, the flash loan functionality might be useless as users won't use it.

[L-01] There should be an upper limit for LBFactory.flashLoanFee

• https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBFactory.sol#L66
• https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBFactory.sol#L474

If the admin sets the flashLoanFee too high, the flash loan functionality might be useless as users won't use it.

[L-02] Missing events for admin functions that change critical parameters

• https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L788
• https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L792

[L-03] Immutable addresses should be 0-checked

• https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBQuoter.sol#L45-L47
• https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L92
• https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBRouter.sol#L57-L59