Trader Joe v2 contest - pashov's results

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/07f84867a018cbfc591f9b5063bf6e59e1f2cb85/src/LBFactory.sol#L474

Vulnerability details

Proof of concept

The code in LBFactory.sol to set the flashloan fee is this

function setFlashLoanFee(uint256 _flashLoanFee) external override onlyOwner {
        uint256 _oldFlashLoanFee = flashLoanFee;

        if (_oldFlashLoanFee == _flashLoanFee) revert LBFactory__SameFlashLoanFee(_flashLoanFee);

        flashLoanFee = _flashLoanFee;
        emit FlashLoanFeeSet(_oldFlashLoanFee, _flashLoanFee);
    }

So the owner can set any flashloan fee any time, without constraints. A malicious or a compromised owner can front-run any flash loans and just set the fee to a high enough number so it drains more value from a flash loan user than he expects.

Impact

This can result in a loss of capital for a flash loan user, but it can only happen if the owner of LBFactory is malicious or compromised, so Medium severity is appropriate.

Recommendation

Add a constraint for setting the flashloan fee, like for example a maximum of 10%, which is 1000 bps.

Lines of code

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

Vulnerability details

Proof of Concept

The swapAVAXForExactTokens function has a built-in mechanism to take more AVAX value in than needed, execute the swap and then return the remaining AVAX. The problem is the returning of the remaining AVAX will always revert because it is coded incorrectly and it is not tested in any unit test

if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, amountsIn[0] - msg.value);

the part amountsIn[0] - msg.value will always revert, because the if statement checks that msg.value is the bigger number.

Impact

This will result in an unexpected revert in most of the usages of the method, so even if you put in 1 more wei of AVAX than needed you won’t be able to execute the swap. Unexpected revert or DoS in most usages should be a Medium severity vulnerability.

Recommendation

Change

if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, amountsIn[0] - msg.value);

to

Remove unneeded variable

LBToken::safeTransferFrom has a not needed variable - _spender it is used only once, so you can just inline it

Scope

https://github.com/code-423n4/2022-10-traderjoe/blob/07f84867a018cbfc591f9b5063bf6e59e1f2cb85/src/LBToken.sol#L137

Add non-zero address checks for address arguments in constructors

Most constructors in the codebase check address arguments for a zero value, but some don’t. Add such checks there.

https://github.com/code-423n4/2022-10-traderjoe/blob/07f84867a018cbfc591f9b5063bf6e59e1f2cb85/src/LBToken.sol#L137

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

Incomplete NatSpec missing to document the return value

In LBFactory::_getPackedFeeParameters the NatSpec is incomplete because it is missing the returns part. Add it

Just use address instead of address payable since you are not using send or transfer

Adding payable to an address variable gives it the ability to call the send and transfer functions of address payable. They are not used in the codebase though so there is no need for the payable keyword there.

Scope

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

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

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

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

Function has a misleading comment

The initialize function in LBPair.sol has the following comment in its NatSpec:

/// It is highly recommended to never call this function directly, use the factory
    /// as it validates the different parameters

but it also has the onlyFactory modifier, while the comment makes you believe the method can be called from a non-factory address which is wrong. Remove the comment

Method does not check if array arguments have the same length

LBPair::burn has two uint256[] parameters - ids and _amounts and they should be of the same length for the code to work as expected. Add a check that their lengths are the same at the beginning of the method.

Change function visibility from public to external

External functions use calldata for method arguments, while public functions use memory, so when possible you should use external instead of public to save gas

Scope:

https://github.com/code-423n4/2022-10-traderjoe/blob/07f84867a018cbfc591f9b5063bf6e59e1f2cb85/src/LBToken.sol#L100

https://github.com/code-423n4/2022-10-traderjoe/blob/07f84867a018cbfc591f9b5063bf6e59e1f2cb85/src/LBToken.sol#L131

https://github.com/code-423n4/2022-10-traderjoe/blob/07f84867a018cbfc591f9b5063bf6e59e1f2cb85/src/LBToken.sol#L149

https://github.com/code-423n4/2022-10-traderjoe/blob/07f84867a018cbfc591f9b5063bf6e59e1f2cb85/src/LBToken.sol#L107

https://github.com/code-423n4/2022-10-traderjoe/blob/07f84867a018cbfc591f9b5063bf6e59e1f2cb85/src/LBQuoter.sol#L134

https://github.com/code-423n4/2022-10-traderjoe/blob/07f84867a018cbfc591f9b5063bf6e59e1f2cb85/src/LBQuoter.sol#L54

Use a more recent version of Solidity

If you use version that is at least 0.8.12 in the codebase you can use  string.concat() instead of abi.encodePacked() in LBFactory::_getPackedFeeParameters