Init Capital Invitational - ladboy233's results

Lines of code

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/a01c4de620be98f9e57a60cf6a82d4feaec54f58/contracts/hook/MarginTradingHook.sol#L479 https://github.com/code-423n4/2024-01-init-capital-invitational/blob/a01c4de620be98f9e57a60cf6a82d4feaec54f58/contracts/hook/MarginTradingHook.sol#L524

Vulnerability details

Impact

MarginTradingHook.sol#updateOrder does not validate order.tokenOut

Proof of Concept

If we take a look at the createOrder function

there are 4 requires input validation check

1. we require collAmount not 0
2. we require initPosId is not 0 and created by user
3. we require the tokenout must be in either marginPos.baseAsset or marginPos.quoteAsset
4. we require the collAmt must be smaller than the collateral amount of marginPos.collPool

but if we take a look at the code updateOrder

we also validate that

1. collAmount not 0
2. we require initPosId is not 0 and created by user
3. the order is active and not fulfilled yet
4. we require the collAmt must be smaller than the collateral amount of marginPos.collPool

which validation is missing?

yes, the tokenOut validation is mssing, user can set arbitrary order.tokenOut when update the order

this is very problematic

1. when fillOrder, the correct account replies on the order.tokenOut parameter when computing amout out repaid shares amount

2. order.tokenOut is what the order fulfiller paid to the order creator recipient

// transfer in repay tokens
IERC20(borrToken).safeTransferFrom(msg.sender, address(this), repayAmt);

// transfer order owner's desired tokens to the specified recipient

IERC20(order.tokenOut).safeTransferFrom(msg.sender, order.recipient, amtOut);

one of the way that order creator can abuse this lack of token address validation is that they can frontrun order filler's fillOrder transaction

1. order filler wants to fill an order, the order.tokenOut is WETH, the order filler expects to pay 0.1 WETH to order creator
2. order creator frontrun fillOrder and calls updateOrder to update tokenOut to WBTC
3. order filler's fillOrder transaction confirmed, he pays order creator in WBTC instead of WETH

order creator collect profits from his recipient address while order filler is at loss

Tools Used

Manual Review

Recommended Mitigation Steps

add the check

_require(_tokenOut == marginPos.baseAsset || _tokenOut == marginPos.quoteAsset, Errors.INVALID_INPUT);

to the function updateOrder

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/a01c4de620be98f9e57a60cf6a82d4feaec54f58/contracts/hook/MarginTradingHook.sol#L387 https://github.com/code-423n4/2024-01-init-capital-invitational/blob/a01c4de620be98f9e57a60cf6a82d4feaec54f58/contracts/hook/MarginTradingHook.sol#L522

Vulnerability details

Impact

Order creator can frontrun order filling by modifying order info

Proof of Concept

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/a01c4de620be98f9e57a60cf6a82d4feaec54f58/contracts/hook/MarginTradingHook.sol#L387

// calculate fill order info
(uint amtOut, uint repayShares, uint repayAmt) = _calculateFillOrderInfo(order, marginPos, collToken);
// transfer in repay tokens
IERC20(borrToken).safeTransferFrom(msg.sender, address(this), repayAmt);
// transfer order owner's desired tokens to the specified recipient
IERC20(order.tokenOut).safeTransferFrom(msg.sender, order.recipient, amtOut);
// repay tokens
_ensureApprove(borrToken, repayAmt);

// @audit
// race condition with liquidation?
IInitCore(CORE).repay(marginPos.borrPool, repayShares, order.initPosId);
// decollateralize coll pool tokens to executor
IInitCore(CORE).decollateralize(order.initPosId, marginPos.collPool, order.collAmt, msg.sender);
// update order status

1. we compute fill order info
2. then we transfer the repay token and transfer the desired token to order.recipient
3. repay borrow pool's debt
4. remove collateral pool's order. creator's collateral

but note that right before fillIngOrder,

the order creator can modify order info, the order creator can update limitPrice_e36 and order.collAmount

order.limitPrice_e36 = _limitPrice_e36;
order.collAmt = _collAmt;

updating triggerPrice_e36 can impact collateral token amount out required for order filler

updating order.collAmt impact order filling repayment amount directly

because there is insufficient slippage control protection for order filler,

order filler may end up filling an order that cost him repaying too debt borrow debt and paying too much collateral pool token

Tools Used

Manual Review

Recommended Mitigation Steps

add slippage protection for order filler, let order filler specify max debt to repay and max token out that he is willing to transferred to the order.recipient

    function fillOrder(uint _orderId, uint256 max_debt_to_repay, uint256 max_token_out) external {
        Order memory order = __orders[_orderId];
        _require(order.status == OrderStatus.Active, Errors.INVALID_INPUT);
        MarginPos memory marginPos = __marginPositions[order.initPosId];
        address collToken = ILendingPool(marginPos.collPool).underlyingToken();
        address borrToken = ILendingPool(marginPos.borrPool).underlyingToken();
        if (IPosManager(POS_MANAGER).getCollAmt(order.initPosId, marginPos.collPool) == 0) {
            order.status = OrderStatus.Cancelled;
            emit CancelOrder(order.initPosId, _orderId);
            return;
        }
        // validate trigger price condition
        _validateTriggerPrice(order, marginPos);
        // calculate fill order info
        (uint amtOut, uint repayShares, uint repayAmt) = _calculateFillOrderInfo(order, marginPos, collToken);
        // transfer in repay tokens
		    
			  // slippage protection for order filler
		    if(repayAmt > max_debt_to_repay) {
			    revert("too much debt repaid");
			  }
			   
			  if(amtOut > max_token_out) {
			    revert("too much collateral token required");
			  }
			  
        IERC20(borrToken).safeTransferFrom(msg.sender, address(this), repayAmt);
        // transfer order owner's desired tokens to the specified recipient
        IERC20(order.tokenOut).safeTransferFrom(msg.sender, order.recipient, amtOut);
        // repay tokens
        _ensureApprove(borrToken, repayAmt);

        IInitCore(CORE).repay(marginPos.borrPool, repayShares, order.initPosId);
        // decollateralize coll pool tokens to executor
        IInitCore(CORE).decollateralize(order.initPosId, marginPos.collPool, order.collAmt, msg.sender);
        // update order status

        // @audit
        // reentrancy?
        __orders[_orderId].status = OrderStatus.Filled;
        emit FillOrder(order.initPosId, _orderId, order.tokenOut, amtOut);
    }

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/a01c4de620be98f9e57a60cf6a82d4feaec54f58/contracts/hook/MarginTradingHook.sol#L422

Vulnerability details

Impact

SwapType.CloseExactOut balance check too strict can be DOSed

Proof of Concept

In CoreCallback function, we have the logic below

uint amtOut = IERC20(swapInfo.tokenOut).balanceOf(address(this));
if (swapInfo.swapType == SwapType.OpenExactIn) {
	// transfer to coll pool to mint
	IERC20(swapInfo.tokenOut).safeTransfer(marginPos.collPool, amtOut);
	emit SwapToIncreasePos(swapInfo.initPosId, swapInfo.tokenIn, swapInfo.tokenOut, amtIn, amtOut);
} else {
	// transfer to borr pool to repay
	uint amtSwapped = amtIn;
	if (swapInfo.swapType == SwapType.CloseExactOut) {
		_require(IERC20(swapInfo.tokenOut).balanceOf(address(this)) == swapInfo.amtOut, Errors.SLIPPAGE_CONTROL);
		amtSwapped -= IERC20(swapInfo.tokenIn).balanceOf(address(this));
	}
	emit SwapToReducePos(swapInfo.initPosId, swapInfo.tokenIn, swapInfo.tokenOut, amtSwapped, amtOut);
}

note the logic

	if (swapInfo.swapType == SwapType.CloseExactOut) {
		_require(IERC20(swapInfo.tokenOut).balanceOf(address(this)) == swapInfo.amtOut, Errors.SLIPPAGE_CONTROL);
		amtSwapped -= IERC20(swapInfo.tokenIn).balanceOf(address(this));
	}

this check

_require(IERC20(swapInfo.tokenOut).balanceOf(address(this)) == swapInfo.amtOut, Errors.SLIPPAGE_CONTROL);

is too strict, malicious uer can DOS the swap by simply perform a tiny swap by frontruning the swap to change the token ratio in the LP pool

for example, the swapInfo.amtOut is 1000 USDC, and swapInfo.tokenOut is USDC

it is difficult to get exact 1000 USDC,

any swap that executes before the swap can make the received USDC amount to 1000.1 USDC or 999. USDC.

then swap transaction revert.

Tools Used

Manual review

Recommended Mitigation Steps

do not use ==, use >=

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-01-init-capital-invitational/blob/a01c4de620be98f9e57a60cf6a82d4feaec54f58/contracts/wrapper/WLpMoeMasterChef.sol#L145 https://github.com/code-423n4/2024-01-init-capital-invitational/blob/a01c4de620be98f9e57a60cf6a82d4feaec54f58/contracts/core/InitCore.sol#L284

Vulnerability details

Impact

LP unwrap / wrap is fully broken if master chef contract has insufficient reward token

Proof of Concept

we need to take a look at the external master chef contract that is not in the control of the init captial protocol team

when deposit / withdraw / harvest / claim, this function _modify is called

which is this code

	if (moeReward > 0) _moe.safeTransfer(account, moeReward);

	if (address(extraRewarder) != address(0)) {
		extraRewarder.onModify(account, pid, oldBalance, newBalance, oldTotalSupply);
	}

as we can see, when deposit / withdraw / harvest, the pending reward is transferred from master chef contract to msg.sender (which is the lp wrapper contract)

when calling extraRewarder.onModify, the reward is transferred from extra rewarder to wrapper contract

But someone needs to transfer the moe token into the master chef to ensure there is sufficient reward token balance

someone needs to transfer the reward token into extraReward contract to ensure there is sufficient reward token balance

in case when there are insufficient reeward token in master chef contract and extraReward,

the code will revert

	if (moeReward > 0) _moe.safeTransfer(account, moeReward);

	if (address(extraRewarder) != address(0)) {
		extraRewarder.onModify(account, pid, oldBalance, newBalance, oldTotalSupply);
	}

suppose the reward accounting update is that the wlp contract is entitled to get 10000 moe token and 100 usdc token as extra reward

but in master chef there are only 9000 token,

attempint to transfer the 10000 moe token will revert

the impact is severe because this revert ,would block lp unwrap and block original lp owner attemps to decollateralize wlp

and make liquidation revert as well

Tools Used

Manual Review

Recommended Mitigation Steps

when regular withdraw failed,

the code should call emergencyWithdraw

this function does not claim reward, but at least this function can ensure withdraw wlp when decollateralize lp or liquidation transaction does not revert.

Assessed type

Token-Transfer

set treasury missing check address(0)

the code should validate _treausry address is not address(0) when setting Treasury in lending pool

function setTreasury(address _treasury) external accrue onlyGovernor {
        treasury = _treasury;
        emit SetTreasury(_treasury);
    }

in fact by default the treasury address is not set,

and accure interest would revert when minting interest to treasury address(0)

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/f6f59bf10f40e06111c66e2fadf652a47919d2f6/contracts/token/ERC20/ERC20Upgradeable.sol#L251

function _mint(address account, uint256 value) internal {
	if (account == address(0)) {
		revert ERC20InvalidReceiver(address(0));
	}
	_update(address(0), account, value);
}

Lack of function way to collateralize and decollateralize using WLP in MarginTradingHook.sol

there lack of functionality to to collateralize and decollateralize using WLP in MarginTradingHook.sol

WLPMoeMasterChef reward token list can be outdated when master chef update extra reward contract

when we updateRewards, the code would append the extra reward token to reward token list

    // add extraReward to __rewardTokens set if there is extraRewarder
        (address extraRewarder, address extraRewardToken) = _getExtraRewarderAndToken(_pid);
        if (extraRewarder != address(0)) __rewardTokens[_pid].add(extraRewardToken);

and the reward token is never removed

but the problem is that the master chef admin can remove extra reward or update extra reward

suppose the reward token list has [MOE, and WETH], WETH is the extra reward

then the extra reward changes to [JOE],

then WETH token data is considered outdated.

if the order.recipient is blocklisted, his order can never be fulfilled

when creating a order in margin trading hook, the code record order.recipient

certain token support blocklisting behavior

https://github.com/d-xo/weird-erc20?tab=readme-ov-file#tokens-with-blocklists

but when filling the order, if the order.recipient is blocklisted, the order can never be fulfilled and fill order always revert

Order cannot be filed in certain case and lack of view function to know if the order can be fulfilled

in margin trading hook, after user open margin position, user can create stop loss or take profits order

but if a user wants to fill the order, it is diffcult to know if the order can be fulfilled

querying whether the order status is active is not enough,

the order fill can revert for many reason,

it could be possible that the order.collAmount is oudated, the user already adjust margin position and remove most of the collateral

it could be possible that order.recipient is blocklisted and token transfer to order.recipient will fail and revert

it could be possible that the user is liquidated and there is no collateral left so when the order filler remove collateral in exchange for filling order

it is recommendated to add a view function to preview if an order can be fulfilled or not

it is also recommended that the order that cannot be filled by user should be canceled

fillOrder may subject to reentrancy

the function fillOrder set order status as filled after external token transfer

https://docs.openzeppelin.com/contracts/3.x/erc777

this opens room for reentrancy if the token is ERC777 token when external user can control the execution flow via the receive hook

the external token transfer is this line of code

// transfer order owner's desired tokens to the specified recipient
       IERC20(order.tokenOut).safeTransferFrom(msg.sender, order.recipient, amtOut);

where order.recipient can re-enter the fillOrder to fill order more than one times

it is recommende to add nonReentrant modifer to the function fillOrder

flashloan does not charge fee

when flashloan, there is no fee charged, so user can flash loan for free

it is recommended to charge flash loan fee

lack of view function for user to query if the flashloan can be used

it is recommended to implement and adopt

https://eips.ethereum.org/EIPS/eip-3156

where there are view function for external user to call to query if the flashloan can be used

   function maxFlashLoan(
        address token
    ) external view returns (uint256);

    /**
     * @dev The fee to be charged for a given loan.
     * @param token The loan currency.
     * @param amount The amount of tokens lent.
     * @return The amount of `token` to be charged for the loan, on top of the returned principal.
     */
    function flashFee(
        address token,
        uint256 amount
    ) external view returns (uint256);