MarginTradingHook.sol#updateOrder does not validate order.tokenOut, allowing order creator maliciously modify order.tokenOut before filling the order

[c4-bot-3]

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

[hansfriese]

As #23 says, the hook should have an approval of the changed tokenOut from an executor. Due to this additional requirement, Medium seems to be more appropriate.

[c4-judge]

hansfriese changed the severity to 2 (Med Risk)

[c4-judge]

hansfriese marked the issue as duplicate of #23

[JeffCX]

I think the severity should be high because it is very common the executor tries to fill a lot of order that have different token,

so it is common that executor approves the hook to spend multiple tokens.

order creator frontrun to update order to different token clearly violate order executor's expectation if order creator update a low value token with high value token before the order is filled.

[hansfriese]

Yes, it's definitely a good finding. After checking again, I think it's worth keeping it as High because users are likely to approve the hook with multiple tokens and the attacker(creator) could select the high-value token among approved ones. Btw I still think #23 explains the impact and relevant requirement better and will keep it as a primary one.

[c4-judge]

hansfriese changed the severity to 3 (High Risk)

[c4-judge]

hansfriese marked the issue as satisfactory