Order can be filled in a very unfavorable condition for order creator if user's borrow pool collateral is liquidated or if the borrower pool debt is already repaid

[c4-bot-2]

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/core/PosManager.sol#L166

Vulnerability details

Impact

Order can be filled in a very unfavorable condition for order creator if user's borrow pool collateral is liquidated

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

the order filler has two things that he should do

1. repay debt in borrow pool
2. transfer the token to order.recipient
3. then he can take collateral

but Order can be filled in a very unfavorable condition for order creator if user's borrow pool collateral is liquidated

if user's borrow pool's collateral is liquidated, most of the debt in borrow pool is repaid by liquidator,

function _calculateRepaySize(Order memory _order, MarginPos memory _marginPos)
    internal
    returns (uint repayAmt, uint repayShares)
{
    uint totalCollAmt = IPosManager(POS_MANAGER).getCollAmt(_order.initPosId, _marginPos.collPool);
    if (_order.collAmt > totalCollAmt) _order.collAmt = totalCollAmt;
    uint totalDebtShares = IPosManager(POS_MANAGER).getPosDebtShares(_order.initPosId, _marginPos.borrPool);
    repayShares = totalDebtShares * _order.collAmt / totalCollAmt; // @audit decimals?
    repayAmt = ILendingPool(_marginPos.borrPool).debtShareToAmtCurrent(repayShares);
}

then the repayShares and repayAmt that order filler needs to repay will be very low and can be nearly to 0 because

    uint totalDebtShares = IPosManager(POS_MANAGER).getPosDebtShares(_order.initPosId, _marginPos.borrPool);

totalDebtShares by calling getPosDebtShares can return very small number, even 0,

because liquidator or someone already repay the debt for borrow pool,

then while the order filler bascially needs to repay no borrow debt and can transfer the token to order recipient and takes user's collateral from collateral pool for free

recall:

the order filler has two things that he should do

1. repay debt in borrow pool
2. transfer the token to order.recipient
3. then he can take collateral

but in the case we described,

repay debt in borrow pool, which is order filler's responsibility, can be avoided and skipped.

one way that order filler can take advantage of this is they can backrun liquidation / repayment to fill order and take collateral pool's collateral immediately within same block

Tools Used

Manual Review

Recommended Mitigation Steps

when borrow pool debt shares is too low, block order filling, or let order filler pays for collateral pool token before letting the order filler remove collateral pool's collateral

Assessed type

Token-Transfer

[c4-judge]

hansfriese marked the issue as primary issue

[c4-sponsor]

fez-init (sponsor) disputed

[JeffCX]

This is invalid

thanks @fez-init for point it out

// 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);

note this line of code

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

if the repayShares / repay amount is less, the amtOut will increase

The amtOut of tokenOut does reflect the reduced repay amt & shares!

because of how _calculateFillOrderInfo is implemented

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

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid