maushish - `market.update` could lead to integration issues in future

[sherlock-admin4]

maushish

medium

market.update could lead to integration issues in future

Summary

market.update could lead to integration issues in future

Vulnerability Detail

When Market.update is called, any parameters except protected = true will perform the following check from the InvariantLib.validate:

if (
    !PositionLib.margined(
        context.latestPosition.local.magnitude().add(context.pending.local.pos()),
        context.latestOracleVersion,
        context.riskParameter,
        context.local.collateral
    )
) revert IMarket.MarketInsufficientMarginError();

This means that even updates which do not change anything (empty order and 0 collateral change) still perform this check and revert if the user's collateral is below margin requirement. Such method to settle accounts is used in MultiInvoker._marketWithdraw:

    /// @notice Withdraws `withdrawal` from `account`'s `market` position
    /// @param market Market to withdraw from
    /// @param account Account to withdraw from
    /// @param withdrawal Amount to withdraw
    function _marketWithdraw(IMarket market, address account, UFixed6 withdrawal) private {
        market.update(account, UFixed6Lib.MAX, UFixed6Lib.MAX, UFixed6Lib.MAX, Fixed6Lib.from(-1, withdrawal), false);
    }

This could lead to settling issues like explained in this audit report

Impact

Future integration issues

Code Snippet

https://github.com/sherlock-audit/2024-05-kwenta-x-perennial-integration-update/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L455

Tool used

Manual Review

Recommendation

Use market.settle.

[arjun-io]

I think the issue is actually here: https://github.com/sherlock-audit/2024-05-kwenta-x-perennial-integration-update/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L462 where _marketSettle uses _marketWithdraw under the hood, instead of actually calling the new settle function

Agreed this is an issue, similar to what was reported in the previous audit report

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/equilibria-xyz/perennial-v2/pull/332

[panprog]

This should be at most low, because this has absolutely no impact unlike the previous report. This update instead of settle is called just before executing the user's order, which will do update anyway. And since order can not be a deposit order, this doesn't change anything in the margin check between those 2 updates.

This means that usage of update or settle in this case doesn't have any difference, maybe except gas usage, which is low at most.

[z3s]

I agree with the lead Watson here. The attack vector mentioned in previous audit, do not have same impact here.

issue is Valid low/Informational.

[maushish]

@panprog Your statement is true, but in the readme it was clearly mentioned: If it weren't, I wouldn't have reported this finding in the first place. According to the sherlock V2 docs: In my issue, the severity would be medium because in future integrations, _market.settle would have potentially lost funds. In normal cases, that would have been an out-of-scope finding, but again, in the 'readme', it was clearly mentioned that out-of-scope issues that pose a threat to future integrations will be considered valid. For this, @z3s, I believe my issue should be escalated.

[panprog]

I see no threat to future integration here. Both update and settle will revert if integrator tries to update position which is below margin, and it's correct expected behaviour. The only difference is that the way it is now it will revert earlier.

[maushish]

I could think of two scenarios already where protocol could easily lose funds if txns gets reverted early:

• If an attacker can manipulate the system to trigger a series of transactions that update the position below the margin and then exploit the reverted _marketSettle to perform another action before the user's transaction is reverted, it could lead to a reentrancy attack.
• The attacker might exploit the temporary position change to manipulate market oracle data for their benefit using flashloans.

  [!NOTE] These are all plausible scenarios put against @panprog's statement, "I see no threat to future integration here," representing the loss of funds by malicious actors in future integrations ( Its in scope).

For the above-stated reasons, I think this issue should be escalated.

[arjun-io]

Without a clear coded up proof of concept on how this current code can lead to any loss of funds I'm inclined to agree that this would be low/informational.

[maushish]

In the current scenario, providing the proof of concept would be a burdensome task because it would involve some assumptions regarding the "future integration" part, but it is completely doable. I can make the POC if @z3s or any other judge requests that they need a POC to define the severity.

[z3s]

In the current scenario, providing the proof of concept would be a burdensome task because it would involve some assumptions regarding the "future integration" part, but it is completely doable. I can make the POC if @z3s or any other judge requests that they need a POC to define the severity.

From Criteria for Issue Validity: "Future issues: Issues that result out of a future integration/implementation that was not mentioned in the docs/README or because of a future change in the code (as a fix to another issue) are not valid issues."

If your future integration scenario is mentioned in the documentation, then it is usable; otherwise, it is not valid. However, if you can define a scenario (with a coded POC) using the current implementation or some documented future integration, then it could be a valid issue.

[panprog]

• If an attacker can manipulate the system to trigger a series of transactions that update the position below the margin and then exploit the reverted _marketSettle to perform another action before the user's transaction is reverted, it could lead to a reentrancy attack.

Not possible for multiple reasons. Updating below margin will revert. How can you exploit reverted _marketSettle? Like I said, if _marketSettle reverts, so will the following update action, so whole transaction will revert anyway. And finally, reentrancy is not possible and has nothing to do with revert or _marketSettle.

• The attacker might exploit the temporary position change to manipulate market oracle data for their benefit using flashloans.

How does the _marketSettle help it?

[maushish]

@panprog arguement:

How does the _marketSettle help it?

Scenario:

• By removing unnecessary withdrawal or collateral update steps, market.settle can avoid creating scenarios where fees or collateral changes could be manipulated.
• This ensures that the state changes relevant to the oracle data are minimized and directly managed, reducing the attack surface for flash loan exploits.
• Hence More secure handling of fees and collateral translates to less risk of oracle data manipulation through indirect means.

[!NOTE] For the above stated reason i would like to request @z3s @panprog to escalate the issue.

[!IMPORTANT] If the issue has not been escalated, there is no need to continue this discussion further, as it will be a waste of time for all the involved contributors. 

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.