Skip to content

Instantly share code, notes, and snippets.

@chaboo

chaboo/report.md Secret

Last active Sep 21, 2022
Embed
What would you like to do?
Kwenta-2 mini audit

Issues

[M-1] freeMargin not checked when charging tradeFee

In MarginBase, _distributeMargin() balances positions according to provided user inputs. It also charges tradeFee by transferring proportional amount of assets from the margin account to the treasury.

if (totalSizeDeltaInUSD > 0) {
    require(
        marginAsset.transfer(
            marginBaseSettings.treasury(),
            (totalSizeDeltaInUSD * marginBaseSettings.tradeFee()) /
                MAX_BPS
        ),
        "MarginBase: unable to pay fee"
    );
}

In MarginBase, committedMargin represents the amount of assets in a margin account reserved for pending (placed but not yet fulfilled) limit orders. These assets are not meant to be used for any other purpose.

The available amount of assets in a margin account to be freely used is calculated by freeMargin():

function freeMargin() public view returns (uint256) {
    return marginAsset.balanceOf(address(this)) - committedMargin;
}

However, when tradeFee is charged in _distributeMargin, freeMargin() is not checked, potentially making the asset balance become smaller than commitedMargin, resulting in underflow errors whenever freeMargin() is called subsequently in:

  • placeOrder (in certain situations)
  • withdraw
  • depositAndModifyPositionForMarket

To recover from this invalid state, it is enough to increase asset balance to amount higher than the committedMargin, which can be done in multiple ways:

  • doing deposit
  • doing margin withdrawal on open position
  • canceling pending order

Questions

  1. Unseen attack vectors or problems with the minimal proxy architecture

    There were no identified issues regarding the overall design and application of minimal proxy architecture. Moreover, the minimal proxy architecture represents an appropriate solution for this project. We did not find any signs of common issues related to this pattern — for example, those related to initialization and access privileges.

  2. Active Market Position(s) Internal State Management

    We confirm issue 4. raised here https://docs.google.com/document/d/13shmRKznmfFlghj-EhQJES2852910FopcTxi3oGTakQ/edit needs to be addressed. We could not identify any other issues related to the internal state management of active market position(s).

  3. Fee Calculation

    There are two potential places where issues may arise due to the inability to collect a fee:

  • When executeOrder, invoked by ops/gelato, fails due to an insufficient eth balance in the margin account to cover the execution fee. Since this type of failure happens during the interaction between the third party and the system, it requires a more thought-out approach. For example, having an additional mechanism to update the margin account owner about execution issues related to their pending limit orders.
  • When distributeMargin, invoked by the margin account owner, fails due to an insufficient margin balance in the account to cover tradeFee. In this situation, the whole transaction is reverted. Therefore, the requested rebalancing will not occur. However, in this case, informing the end user about the outcome is relatively easy.
  1. Arbitrary Code Execution via .call()
  • Through the withdrawEth function, the owner may trigger an arbitrary code execution via .call(). However, we have not been able to identify a way to exploit this capability with the current state of the codebase. So, at the moment, this instance of an arbitrary code execution does not lead to a security vulnerability.
  • A second point of origin for potential arbitrary code execution via .call() is in the OpsReady _transfer function. _transfer is only used in executeOrder guarded by onlyOps, and therefore only accessible by ops. This would generally represent a potential security risk due to re-entrancy. However, executeOrder properly applies the CEI pattern (validOrder check; followed by removal of order record; before the external call), which prevents re-entrant executions. Cross method re-entrancy is also impossible because executeOrder is the only function accessible to the ops account. So at the moment (with the current codebase), this origin of arbitrary code execution does not introduce security issues.
  1. Potential gas optimizations
  1. Low-level issues
  • MarginAccountFactory has no way of enumerating the accounts created.
  • There is no mechanism to confirm a MarginBase instance is created by an authorized MarginAccountFactory.
  1. Ops is not immutable because it cannot be in the minimal proxy architecture unless it is hardcoded in the master instance
  • The minimal proxy constructor cannot have arguments.
  • Therefore, ops cannot be set in the constructor, but only within the initializer, which is called once the master instance is cloned. This prevents ops from being declared immutable.
  1. Spec misalignment
  • In the KIP-18, it is specified that the fee is calculated based on the total margin balanced. However, the implementation calculates the fee based on the total size change.

Further Research

Items that we didn’t get to, but would look into during a full audit.

  1. What are gelato execution guarantees, and are they acceptable for the Kwenta use case? Are assets in limit orders lucrative enough for a Gelato Executor agent to act maliciously? Is this malicious behavior detectable? How can it be monitored? These are questions to investigate in a follow-up study of Gelato.
  2. An additional investigation may be necessary to verify what happens to the pending orders that are not executed because of an insufficient eth balance. Are they retried or never attempted again? And how is the margin account owner made aware of the outcome, whatever that outcome is?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment