Trader Joe v2 contest - ladboy233's results

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBToken.sol#L237 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L663 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L829 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L894 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L870 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L871 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L850

Vulnerability details

Impact

Fee accounting is broken when LBToken is burned because of the incorrect function call of _beforeTokenTransfer

When LBPosition Token is transfered, minted or burned, the _beforeTokenTransfer hook is called to collect and update the fees.

/// @notice Collect and update fees before any token transfer, mint or burn
/// @param _from The address of the owner of the token
/// @param _to The address of the recipient of the  token
/// @param _id The id of the token
/// @param _amount The amount of token of type `id`
function _beforeTokenTransfer(
	address _from,
	address _to,
	uint256 _id,
	uint256 _amount
)

When the LBPosition token is minted, we call _beforeTokenTransfer (this is correct)

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

_beforeTokenTransfer(address(0), _account, _id, _amount);

however, when the LBPosition token is burned, we also call (this is incorrect)

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

_beforeTokenTransfer(address(0), _account, _id, _amount);

we should call

_beforeTokenTransfer(_account, address(0), _id, _amount);

instead because given the implementation of _beforeTokenTransfer, fee should be deducted from address(from) and added to address(to)

if (_from != _to) {
	if (_from != address(0) && _from != address(this)) {
		uint256 _balanceFrom = balanceOf(_from, _id);
		_cacheFees(_bin, _from, _id, _balanceFrom, _balanceFrom - _amount);
	}

	if (_to != address(0) && _to != address(this)) {
		uint256 _balanceTo = balanceOf(_to, _id);
		_cacheFees(_bin, _to, _id, _balanceTo, _balanceTo + _amount);
	}
}

when burning LBPosition, the fee is added to the account owner again instead of deducting from the token owner owner.

Proof of Concept

Let's track the function call.

When burn is called,

we call

_beforeTokenTransfer(address(0), _account, _id, _amount);

then we call

_cacheFees(_bin, _to, _id, _balanceTo, _balanceTo + _amount);

then we call

_updateUserDebts(_bin, _user, _id, _newBalance);

the _newBalance is _balanceTo + _amount

then we call

uint256 _debtX = _bin.accTokenXPerShare.mulShiftRoundDown(_balance, Constants.SCALE_OFFSET);
uint256 _debtY = _bin.accTokenYPerShare.mulShiftRoundDown(_balance, Constants.SCALE_OFFSET);

_accruedDebts[_account][_id].debtX = _debtX;
_accruedDebts[_account][_id].debtY = _debtY;

as we can see, the _debtX and _debtY is incorrectly updated when the token is burned.

then we we getPendingFee, user get less fee because the _debts.debtX and _debt.debtY incorrectly inflated.

Debts memory _debts = _accruedDebts[_account][_id];

amountX = _bin.accTokenXPerShare.mulShiftRoundDown(_balance, Constants.SCALE_OFFSET) - _debts.debtX;
amountY = _bin.accTokenYPerShare.mulShiftRoundDown(_balance, Constants.SCALE_OFFSET) - _debts.debtY;

Tools Used

Foundry, Manual Review

Recommended Mitigation Steps

We recommend change from

_beforeTokenTransfer(address(0), _account, _id, _amount);

to

_beforeTokenTransfer(_account, address(0), _id, _amount);

in the burn function

Lines of code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBFactory.sol#L63 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBFactory.sol#L474 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L428 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L430 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L431 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L447 https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBPair.sol#L448

Vulnerability details

Impact

Compromised admin can charge unreasonable amount of flashloan fee because there is no upper limit for flashloan fee setting.

The flashloan fee is set in the constructor

constructor(address _feeRecipient, uint256 _flashLoanFee) {
	_setFeeRecipient(_feeRecipient);
	flashLoanFee = _flashLoanFee;
	emit FlashLoanFeeSet(0, _flashLoanFee);
}

and the flashing fee can be adjusted via

/// @notice Function to set the flash loan fee
/// @param _flashLoanFee The value of the fee for flash loan
function setFlashLoanFee(uint256 _flashLoanFee) external override onlyOwner {
	uint256 _oldFlashLoanFee = flashLoanFee;

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

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

The flashloan is used in the function flashloan in LBPair.sol

function flashLoan(
	address _to,
	uint256 _amountXOut,
	uint256 _amountYOut,
	bytes calldata _data
) 

inside the function, the flashloan borrower need to repay borrowed amount + the flashloan fee

uint256 _fee = factory.flashLoanFee();
FeeHelper.FeesDistribution memory _feesX = _fp.getFeeAmountDistribution(_getFlashLoanFee(_amountXOut, _fee));
FeeHelper.FeesDistribution memory _feesY = _fp.getFeeAmountDistribution(_getFlashLoanFee(_amountYOut, _fee));

the validation calls stack is

_feesX.flashLoanHelper(_pairInformation.feesX, tokenX, _reserveX);
_feesY.flashLoanHelper(_pairInformation.feesY, tokenY, _reserveY);

_feesX.flashLoanHelper -> _token.received(_reserve, _totalFees)

inside the _token.received, token.balanceOf(address(this)) - _internalBalance will revert in underflow if the borrow amount + fee is not paid.

function received(
  IERC20 token,
  uint256 reserve,
  uint256 fees
) internal view returns (uint256) {
  uint256 _internalBalance;
  unchecked {
  	_internalBalance = reserve + fees;
  }
  return token.balanceOf(address(this)) - _internalBalance;
}

The fee amount is calculated inside the LBPair.sol

function _getFlashLoanFee(uint256 _amount, uint256 _fee) internal view returns (uint256) {
	return (_amount * _fee) / Constants.PRECISION;
}

given the formula: amount of borrowed * fee setting by protocol / by constant 10 ** 18,

the protocol can charge the unreasonable amount of fee (such as 80% of the borrow amount) to the borrower because there is no upper limit for flashloan fee setting.

A compromised admin can set the flashloan fee to a very large number, then either user will not use the flashloan function or they need to pay unreasonable amount of fee.

Proof of Concept

We import console log for debugging

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/test/LBPair.FlashLoans.t.sol#L5

import "forge-std/console.sol";

and we add the POC below

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/test/LBPair.FlashLoans.t.sol#L29

function testFlashloan_Fee_NoLimit_POC() public {
	(uint256[] memory _ids, , , ) = addLiquidity(100e18, ID_ONE, 9, 5);
	uint256 amountXBorrowed = 10e18;
	uint256 amountYBorrowed = 10e18;

	// Paying for fees
	token6D.mint(address(borrower), 1e18);
	token18D.mint(address(borrower), 1e18);

	vm.expectEmit(false, false, false, false);
	emit CalldataTransmitted();

	factory.setFlashLoanFee(10e18);

	borrower.flashBorrow(10e6, 0);

	(uint256 feesForDevX, ) = pair.pendingFees(DEV, _ids);
	console.log(feesForDevX);
}

we charge unreasonable amount of fee

factory.setFlashLoanFee(10e18);

we run the test

forge test -vv --match testFlashloan_Fee_NoLimit_POC

the running result is

Running 1 test for test/LBPair.FlashLoans.t.sol:LiquidityBinPairFlashLoansTest
[PASS] testFlashloan_Fee_NoLimit_POC() (gas: 1689758)
Logs:
  89999999

the fee 89999999 is even larger than borrow amount 10e6, which is not reasonable.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

We recommend the project add the upper limit for flashloan fee seetting

/// @notice Function to set the flash loan fee
/// @param _flashLoanFee The value of the fee for flash loan
function setFlashLoanFee(uint256 _flashLoanFee) external override onlyOwner {
   if(_flashLoanfee > flashloanFeeLimit) {
		revert LBFactory__FlashfloanFeeTooLarge(_flashLoanFee)
	}
	uint256 _oldFlashLoanFee = flashLoanFee;

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

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

and inside constructor

constructor(address _feeRecipient, uint256 _flashLoanFee) {
	_setFeeRecipient(_feeRecipient);
	setFlashLoanFee(_flashLoanFee)
}