MCDEX Mai Protocol V2

Description

UPDATE: The client provided the following statement: The perpetual proxy is now replaced by a generic call proxy. As we discussed in telegram, the PerpetualProxy is a address holder for AMM margin account. It simplify the upgrade of AMM contract which we may upgrade relatively frequent, so we decide not to remove it.

PerpetualProxy is a forwarding contract that mirrors the Perpetual interface and provides a few wrappers for Perpetual functions. While the inclusion of the word Proxy implies that PerpetualProxy is a standard delegatecall proxy, its operations all use call. This means that Perpetual holds the state used by PerpetualProxy, with PerpetualProxy’s only state being a single address pointing to Perpetual.

While there is evidence within MCDEX’s docs that PerpetualProxy originally held additional state, this is no longer the case. However, PerpetualProxy does still implement additional access-control logic that may be a holdover from a previous version. Namely, it includes the onlyAMM modifier, which restricts function call access to the AMM contract. Because PerpetualProxy does not hold state, this modifier must query Perpetual:

code/contracts/proxy/PerpetualProxy.sol:L13-L18

IPerpetual perpetual;

modifier onlyAMM() {
    require(msg.sender == address(perpetual.amm()), "invalid caller");
    _;
}

Note that PerpetualProxy is not an abstract calldata forwarder; it does not include a fallback function that forwards msg.data to Perpetual. Rather, each of the functions PerpetualProxy needs to call are a part of its own interface. Many of these functions are restricted to the amm contract, by use of the onlyAMM modifier:

code/contracts/proxy/PerpetualProxy.sol:L110-L124

function setBrokerFor(address guy, address broker) public onlyAMM {
    perpetual.setBrokerFor(guy, broker);
}

function depositFor(address guy, uint256 amount) public onlyAMM {
    perpetual.depositFor(guy, amount);
}

function depositEtherFor(address guy) public payable onlyAMM {
    perpetual.depositEtherFor.value(msg.value)(guy);
}

function withdrawFor(address payable guy, uint256 amount) public onlyAMM {
    perpetual.withdrawFor(guy, amount);
}

Of course, Perpetual and PerpetualProxy do not share state, and Perpetual’s functions can be called directly. This separation of logic and state has resulted in two separate access control implementations: the onlyAMM modifier in PerpetualProxy, and an onlyWhitelisted modifier in Perpetual. In order to successfully restrict a function’s access to the AMM contract only, both modifiers must be employed:

• PerpetualProxy ensures the caller is the AMM contract via onlyAMM, then forwards the call to Perpetual:

code/contracts/proxy/PerpetualProxy.sol:L110-L112

function setBrokerFor(address guy, address broker) public onlyAMM {
    perpetual.setBrokerFor(guy, broker);
}

• Perpetual then needs to check that the caller (PerpetualProxy) is whitelisted via onlyWhitelisted:

code/contracts/perpetual/Perpetual.sol:L64-L66

function setBrokerFor(address guy, address broker) public onlyWhitelisted {
    setBroker(guy, broker, globalConfig.brokerLockBlockCount());
}

The onlyWhitelisted modifier, which is pulled from OpenZeppelin’s WhitelistedRole contract, allows multiple whitelisted addresses. In the case of Perpetual, PerpetualProxy is not the only whitelisted address: the Exchange contract is also whitelisted. Additionally, the whitelist admin role in OpenZeppelin’s WhitelistAdminRole contract allows additional whitelisted addresses to be added, each of which would have the same permissions as Exchange and PerpetualProxy.

Conclusion

The two-contract system is complicated, which is compounded by the use of the whitelist access control system. Technically, Exchange has the same access to Perpetual as the AMM contract, and vice-versa. Should issues be found or introduced in either Exchange or AMM that allow for arbitrary external calls, several components of the system may break or be tampered with. Further additions to the list of whitelisted addresses or whitelisted admins may have similar consequences.

Recommendation

• Remove PerpetualProxy entirely, as it no longer serves a purpose

• Implement the onlyAMM modifier within Perpetual, and replace onlyWhitelisted in Perpetual with onlyAMM where applicable

• Remove onlyWhitelisted in Perpetual, and implement an onlyExchange modifier where applicable

• Review system roles and permissions, and ensure that each contract is only given the minimum level of access needed to function effectively

Description

One factor that significantly introduces complexity to the smart contract system is the excessive use of signed integers for convenience and to encode implicit logic.

In practice, this degrades code readability, auditability, and security as it breaks assumptions humans might have formed based on the variable’s use or name.

Examples

• variables declared as signed int that must not be negative: e.g., insuranceFundBalance

The variable is declared as a signed integer. However, the value of the insurance fund should never be allowed to be negative. In fact, it cannot be negative unless someone withdraws more funds than available. Since it is a signed integer it can theoretically be negative (permanently or for a short amount of time potentially during reentrant calls). To counter that, special care must be taken and explicit checks are added while simply falling back to declaring the variable uint would have avoided the necessity of adding more complexity.

code/contracts/perpetual/Position.sol:L15-L15

int256 public insuranceFundBalance;

code/contracts/perpetual/Perpetual.sol:L192-L202

function depositEtherToInsuranceFund() public payable {
    require(!isTokenizedCollateral(), "ether not acceptable");
    require(msg.value > 0, "invalid amount");

    int256 wadAmount = depositToProtocol(msg.sender, msg.value);
    insuranceFundBalance = insuranceFundBalance.add(wadAmount);

    require(insuranceFundBalance >= 0, "negtive insurance fund");

    emit UpdateInsuranceFund(insuranceFundBalance);
}

• multiple declarations of the same const for different signedness

code/contracts/liquidity/AMM.sol:L17-L18

uint256 private constant ONE_WAD_U = 10**18;
int256 private constant ONE_WAD_S = 10**18;

• implicit logic based on the signedness of an integer: fee

If the fee is positive, the amount is sent from guy -> devAddress. If the fee is negative, the amount is sent from devAddress -> guy.

Let alone, that this mechanism shifts responsibility to verify the sanity of system and order parameters to the entity matching orders (usually the broker) as the signed integer order fee rate defines if the broker or the trader pays fees.

code/contracts/exchange/Exchange.sol:L200-L212

int256 hard = price.wmul(openedAmount).toInt256().wmul(feeRate);
int256 soft = price.wmul(closedAmount).toInt256().wmul(feeRate);
int256 fee = hard.add(soft);
address devAddress = perpetual.devAddress();
if (fee > 0) {
    int256 available = perpetual.availableMargin(guy);
    require(available >= hard, "dev margin");
    fee = fee.min(available);
    perpetual.transferCashBalance(guy, devAddress, fee.toUint256());
} else if (fee < 0) {
    perpetual.transferCashBalance(devAddress, guy, fee.neg().toUint256());
    require(perpetual.isSafe(devAddress), "dev unsafe");
}

code/contracts/exchange/Exchange.sol:L93-L95

// trading fee
int256 takerTradingFee = amount.wmul(price).toInt256().wmul(takerOrderParam.takerFeeRate());
claimTradingFee(perpetual, takerOrderParam.trader, takerTradingFee);

• transferBalance can never be called with a negative value but wadAmount is signed int.

code/contracts/perpetual/Collateral.sol:L140-L144

function transferBalance(address from, address to, int256 wadAmount) internal {
    if (wadAmount == 0) {
        return;
    }
    require(wadAmount > 0, "bug: invalid transfer amount");

that’s also why explicit conversions to int256 are required:

code/contracts/perpetual/Perpetual.sol:L313-L316

function transferCashBalance(address from, address to, uint256 amount) public onlyWhitelisted {
    require(status != LibTypes.Status.SETTLING, "wrong perpetual status");
    transferBalance(from, to, amount.toInt256());
}

Recommendation

Rework the smart contract system design and declare signed integers only where they are absolutely needed. Refrain from declaring signed integers out of convenience when used with arithmetical operations. Refrain from encoding logic - like the direction of funds flow - into the signedness of the value and make it explicit instead. Clearly explain why variables are signed and reflect the type of arguments and statevars used in the method’s docstring. The more explicit the code is and the less complex it is, the easier it is to verify security assumptions.

Description

UPDATE: The client provided the following statement: Document strings added.

Mai V2 lacks inline code documentation describing the purpose and relationships of source-units, their contracts, methods, and variables. Additionally, supporting documentation is frequently out-of-date, and many interfaces, roles, states, and permissions are missing entirely.

Recommendation

• Rather than duplicating function description in external documentation, provide inline documentation using Solidity’s natspec format, as this will be easier to maintain.

• Provide supporting inline comments for critical functionality:

  • Outline why certain values are used and what purpose they serve.
  • Describe acceptable ranges for inputs, outputs, and intermediary calculations.
  • Elaborate on security concerns for critical methods and make your developers or external reviewers aware of any functions that require special attention due to their risk profile in the system.

• Improve, update, and complete the Mai V2 specification:

  • Ensure it is up-to-date at all times and implement the logic as specified without any deviations (e.g. deviation between Solidity math implementation and specification pseudocode).
  • Include a security discussion in the specification and inform users, developers, and reviewers of the risks attached to the system or components that require special attention.

Examples

The following non-exhaustive list provides an overview of various inconsistencies encountered during review. We highly recommend reviewing all documentation for accuracy and completeness as additional issues are likely to exist.

Inconsistent, unclear or insufficient explanation

• Perpetual Wrong state requirements that have since been corrected

mcdexio/documents@f5c1bd7

• Perpetual Wrong state requirement NORMAL for withdraw while the code checks for !SETTLING which resolves to NORMAL and SETTLED

https://github.com/mcdexio/documents/blob/0a44d7ec48e09e2d229a3c5b77501235d4de82b3/en/perpetual-interfaces.md

• Perpetual inaccurate function signature

totalSize(Side side) should be totalSize(LibTypes.Side side) (and multiple other occurrences). Keep the function signature and types as accurately updated with the codebase.

https://github.com/mcdexio/documents/blob/0a44d7ec48e09e2d229a3c5b77501235d4de82b3/en/perpetual-interfaces.md

• AMM internal specification inconsistencies

  • Missing sources for mathematical calculations
  • Most of the Variable and Method names do not reflect actual names in code: GovPoolFeeRate, PoolAvailableMargin, and others.
  • createPool does not mention that it can only be called once otherwise initFunding bails
  • createPool does not mention the state requirement NORMAL
  • buyFromPool does not exist and should be buyFrom. It is also missing the state requirement NORMAL.
  • buyFromPool inconsistent requirement BlockTime < DeadLine which actually is require(getBlockTimestamp() <= deadline, "deadline exceeded"); in code.
  • buyFromPool does not specify a minimum amount.
  • buyFromPool states The trader buy/long. Can be called by anyone. while it can only be called if the caller set the broker to perpetualProxy (which is mentioned as a confusing requirement broker == LiquidityPool).
  • buyFromPool does not mention the lotsize
  • sellFromPool similar inconsistencies to buyFromPool
  • AddLiquidity does not mention he state NORMAL as a requirement.
  • AddLiquidity unclear statement The unit of "Amount" is contract.
  • RemoveLiqudity does not mention that up to a lotsize of balance might be lost
  • UpdateIndex does not mention that the caller might be awarded a premium
  • funding states isEmergency as a requirement which is not state.
  • funding does not check the state requirement and can be called at any time.
  • funding steps are inconsistent with the code. E.g. when lastFundingTime==0 forceFunding() just returns and does not set the LastFundingTime to BlockTime.
  • funding duplicate definition and deviating specification of formulas (even though they are the same): v0 = LastEMAPremium; vt = (LastEMAPremium - LastPremium) * Pow(GovEMAAlpha2, n) + LastPremium vs. - v0 = LastEMAPremium; vt = (LastEMAPremium - LastPremium) * Pow(1 - GovEMAAlpha, n) + LastPremium (here)

https://github.com/mcdexio/documents/blob/b94b98a806d29d7ce135e1011b094868e07eeb5d/en/internal-amm.md#createpoolamount

• depositToInsuranceFund unclear who would deposit to an insurance fund that can be drained by admins at any time.

• LibTypes.Side should add a description for when and how FLAT is used.

https://github.com/mcdexio/documents/blob/0a44d7ec48e09e2d229a3c5b77501235d4de82b3/en/perpetual-interfaces.md

• Exchange vague requirement for amounts array

Length of parameter ‘amounts’ should equal to the length of ‘makerOrderParams’.

https://github.com/mcdexio/documents/blob/0a44d7ec48e09e2d229a3c5b77501235d4de82b3/en/perpetual-interfaces.md

• Perpetual admin functionality vague state requirements

Perpetual.beginGlobalSettlement: Enter the “Emergency” status with a “settlement price”. In this status, all trades and withdrawals will be disabled until “endGlobalSettlement”

Unclear specification. SETTLING is referred to as EMERGENCY mode but it is not mentioned here. Stick to one distinct state description and use it throughout the specification and in code.

Perpetual.setCashBalance: Modify account.cashBalance. Can only be called in the “global settlement” status

Inconsistent and unclear use of state name global settlement. This should state that this method can only be used in EMERGENCY or SETTLING mode. Stick to one name and use it throughout the specification and in code.

Perpetual.endGlobalSettlement: Enter the “global settlement” status. In this status, all traders can withdraw their MarginBalance

Unclear what state is being entered right now. This should state that SETTLED mode is entered.

Perpetual.withdrawFromInsuranceFund: Withdraw collateral from insurance fund. Typically happen in the “global settlement” status

Vague description of when this is allowed to be used when it basically can be called by an admin at any time as there is no state requirement.

https://github.com/mcdexio/documents/blob/0a44d7ec48e09e2d229a3c5b77501235d4de82b3/en/perpetual-admin-functions.md

• Unclear if it is by design that parameters can be changed by an admin at any time (including upgrading the system)

https://github.com/mcdexio/documents/blob/0a44d7ec48e09e2d229a3c5b77501235d4de82b3/en/perpetual-admin-functions.md

• GlobalConfig specification does not mention that there can be multiple admins, admins can add other admins, and whitelist accounts. Whitelisted accounts are not used with this contract.

https://github.com/mcdexio/documents/blob/0a44d7ec48e09e2d229a3c5b77501235d4de82b3/en/perpetual-admin-functions.md

• Contracts that provide admin functionality should clearly state who is assigned admin powers initially (deployed), whether the deployed keeps its admin role or renounces it, what other accounts get roles assigned (admin and whitelisted) to allow uses to audit a specific setup of the system. Note that processes need to be in place to manage privileged accounts (e.g. remove admins when they are compromised, remove privileges when they are no longer used or components are being upgraded)

https://github.com/mcdexio/documents/blob/0a44d7ec48e09e2d229a3c5b77501235d4de82b3/en/perpetual-admin-functions.md

• AMM describe the liquidity provider user journey.

Create pool must be called first and can only be called once. A pool cannot be created for an empty amount. A pool must exist for others to provide liquidity. A pool is not created automatically if none exists. Liquidity providers cannot provide zero amount.

The function descriptions should outline the requirements to call these methods more clearly. E.g. buy, sell, addLiquidity, removeLiquidity can only be called if the caller set the broker to PerpetualProxy.

• Clearly define and explain the operating values and boundaries for configuration parameters

https://github.com/mcdexio/documents/blob/b94b98a806d29d7ce135e1011b094868e07eeb5d/en/internal-amm.md#governance

• Perpetual outlines three states Normal, Emergency and GlobalSettled but the implementation refers to these states as Normal, Settling and Settled.

https://github.com/mcdexio/documents/blob/b94b98a806d29d7ce135e1011b094868e07eeb5d/en/internal-perpetual.md

• Perpetual methods state requirement isEmergency==FALSE while the implementation checks status==NORMAL.

https://github.com/mcdexio/documents/blob/b94b98a806d29d7ce135e1011b094868e07eeb5d/en/internal-perpetual.md

• Perpetual implements no method buy/sell (AMM does)

https://github.com/mcdexio/documents/blob/b94b98a806d29d7ce135e1011b094868e07eeb5d/en/internal-perpetual.md

• Perpetual.liquidate does not state that method can only be called in status.NORMAL or status.SETTLING

https://github.com/mcdexio/documents/blob/b94b98a806d29d7ce135e1011b094868e07eeb5d/en/internal-perpetual.md

Initially Missing but since then updated

• A description for socialLoss and fundingLoss was not present and was added towards the 2nd half of the audit.

mcdexio/documents@9859727

Description

Combined setter methods degrade readability and code maintainability and are prone to errors, especially when one setter method is used to store different types of values.

Examples

• GlobalConfig

code/contracts/global/GlobalConfig.sol:L18-L27

function setGlobalParameter(bytes32 key, uint256 value) public onlyWhitelistAdmin {
    if (key == "withdrawalLockBlockCount") {
        withdrawalLockBlockCount = value;
    } else if (key == "brokerLockBlockCount") {
        brokerLockBlockCount = value;
    } else {
        revert("key not exists");
    }
    emit UpdateGlobalParameter(key, value);
}

• AMMGovernance

code/contracts/liquidity/AMMGovernance.sol:L22-L42

function setGovernanceParameter(bytes32 key, int256 value) public onlyWhitelistAdmin {
    if (key == "poolFeeRate") {
        governance.poolFeeRate = value.toUint256();
    } else if (key == "poolDevFeeRate") {
        governance.poolDevFeeRate = value.toUint256();
    } else if (key == "emaAlpha") {
        require(value > 0, "alpha should be > 0");
        governance.emaAlpha = value;
        emaAlpha2 = 10**18 - governance.emaAlpha;
        emaAlpha2Ln = emaAlpha2.wln();
    } else if (key == "updatePremiumPrize") {
        governance.updatePremiumPrize = value.toUint256();
    } else if (key == "markPremiumLimit") {
        governance.markPremiumLimit = value;
    } else if (key == "fundingDampener") {
        governance.fundingDampener = value;
    } else {
        revert("key not exists");
    }
    emit UpdateGovernanceParameter(key, value);
}

Recommendation

Implement individual setter methods for different values, especially when setting different value types.

With the current architecture multiple calls to set*Parameter are needed to initialize the contract. Consider adding a constructor or method that allows to initially set all the value with one call to save gas.

Description

UPDATE: The client provided the following statement: Fixed. NOTE: The assessment team would like to note that the status names still do not match the names used in the specification (e.g. GlobalSettled)

Consider renaming LibTypes.Status.SETTLING to LibTypes.Status.EMERGENCY to accurately reflect what it is being used for. The status names currently do not match the status mentioned in the specification.

code/contracts/reader/ContractReader.sol:L59-L60

params.isEmergency = perpetual.status() == LibTypes.Status.SETTLING;
params.isGlobalSettled = perpetual.status() == LibTypes.Status.SETTLED;

Description

The Mai V2 contracts do not adhere to consistent naming conventions. Because of the intricate mathematical operations and accounting inherent to the protocol, this has resulted in a significant increase in code complexity.

Addressing the underlying problem will require careful thought and consideration. Generally, the goal should be to make the contracts as readable as possible. The following list of recommendations should serve as a basis for more a more clear, consistent naming scheme within the Mai V2 contracts:

Recommendation

• Signed and unsigned variables should be distinguished: uint uVarName vs int iVarName

• Wad-denominated values should be distinguished: uint wadVarName vs uint rawVarName

• Signed and unsigned math libraries should have different names for the operations they support: uVarName.add(...) vs iVarName.iAdd(...)

• internal and private functions should be distinguished: function _helperMethod() internal;

• Functions that change state should never be prefixed with “get”.

• For example: AMM.getBuyPrice and AMM.getSellPrice

• Many functions act as a wrapper for calls to either AMM.funding or Perpetual.markPrice (which calls AMM.funding eventually). This makes it very difficult to determine where state changes occur in the contracts. Instead of using wrapper functions, ensure that each call to funding is explicit.

• For example, avoid using methods like AMM.currentFairPrice, which calls funding() (a state changing function) then lastFairPrice() (a view getter).

Description

The contract system mixes raw values with values denominated in wads. Reading the code, it is not always immediately clear if a method requires or processes a wad value, or a raw value.

It is therefore recommended to prefix/suffix variables with their respective or expected type to increase code readability and maintainability and reduce the risk of variables being used in the wrong numerical context.

As one example, it is not immediately clear from calling the method wpow that x is a wad value, and n is a raw value. By renaming x to x_wad, its context would be much more visible.

code/contracts/lib/LibMath.sol:L103-L116

// x ^ n
// NOTE: n is a normal integer, do not shift 18 decimals
// solium-disable-next-line security/no-assign-params
function wpowi(int256 x, int256 n) internal pure returns (int256 z) {
    z = n % 2 != 0 ? x : _WAD;

    for (n /= 2; n != 0; n /= 2) {
        x = wmul(x, x);

        if (n % 2 != 0) {
            z = wmul(z, x);
        }
    }
}

Description

According to the specification, the contract system can be in one of three states:

• Normal (default)
• Emergency
• GlobalSettled.

By default, after deployment, the system is in state Normal indicating normal operation even though the contract may not yet be fully set up for use as none of the governance settings are initialized. Uninitialized settings can lead to the system being operated in an unspecified setting and may cause all sorts of issues and side-effects.

For example, PerpetualGovernance is part of Perpetual. It allows a whitelisted admin to set critical system parameters like the initialMarginRate or lotSize which is not allowed to be zero. However, right after deployment it is uninitialized and is, therefore, going to return a zero value which is not within specification. Since an admin is actively required to set critical parameters on a one-by-one basis it may happen that initialiMarginRate is never set and stays at a zero rate. This is just an example and should be easily detectable if the required processes are in place but it should be noted that there is no requirement to initialize the system with a sane configuration before it is set to Normal state.

code/contracts/perpetual/PerpetualGovernance.sol:L41-L44

governance.initialMarginRate = value.toUint256();
require(governance.initialMarginRate > 0, "require im > 0");
require(governance.initialMarginRate < 10**18, "require im < 1");
require(governance.maintenanceMarginRate < governance.initialMarginRate, "require mm < im");

The general recommendation for reasonably complex systems that require parameterization before they can be set to normal operation mode is to

• provide sane (according to the specification) default values as part of the deployment process when executing the contract’s constructor.
• introduce a one-way Setup phase. Make this the first phase that is active by default when deploying the contract (e.g. the first phase in the enum). Provide an interface for others to poll the status of the system to indicate that the system is not yet ready for normal use. In many cases it can make sense to disable certain functionality or pause the complete contract during the setup phase to reject any unwanted user interaction and minimize the risk of losses. Configure and parameterize the system as needed, perform testing to verify that it is set up according to the system deployment plan, and safely transfer it to Normal state indicating that it is now safe for use by others.
• do not allow to configure critical system parameters while the contract is actively being used as this can introduce unforeseeable side-effect.

Description

LibMath.sol provides two libraries LibMathSigned and LibMathUnsigned. The source unit does not contain any hints or references to the original source from where the code was taken from.

Make sure to use only security audited versions of third-party libraries with your codebase. If possible declare third-party libraries with the project’s dependencies instead of copying them into your project or copying methods into new libraries. Copies of general-purpose libraries or methods may easily get outdated and often end up not being updated. This might leave the project vulnerable to security issues that are fixed in the upstream version already. Add comments for code that was taken else-where and install a process that checks 3rd party dependencies for security updates.

LibMath.sol contains source code from:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/math/SignedSafeMath.sol
• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/SafeCast.sol
• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/math/Math.sol

Description

UPDATE: The client provided the following statement: Just for explanation. Some of the events are serving for our off-chain detector, so we’ll leave it asis.

Consider removing unnecessary events like the one in GlobalConfig that just indicates that the contract has been deployed.

code/contracts/global/GlobalConfig.sol:L14-L16

constructor() public {
    emit CreateGlobalConfig();
}

Description

It should be noted that ABIEncoderV2 is an experimental feature in Solidity 0.5.x. With 0.6.0 the ABIEncoderV2 is not considered experimental anymore (see solidity changelog). However, even the more recent versions of solidity list bug-fixes for the encoder and it should, therefore, be tested very thoroughly with the contract system.

To improve readability it is recommended to only specify ABIEncoderV2 for source units that actually make use of it. For example, the following files unnecessarily declare the feature:

code/contracts/lib/LibSignature.sol:L2-L2

pragma experimental ABIEncoderV2; // to enable structure-type parameter

code/contracts/lib/LibTypes.sol:L2-L2

pragma experimental ABIEncoderV2; // to enable structure-type parameter

Description

UPDATE: The client provided the following statement: Fixed.

Multiple definitions of types can be difficult to maintain and lead to security issues if the type is undergoing changes but change is not made for all definitions. Defining the same struct should, therefore, be avoided. Import the type from the respective source (e.g. LibTypes).

code/contracts/lib/LibSignature.sol:L4-L11

library LibSignature {
    enum SignatureMethod {ETH_SIGN, EIP712}

    struct OrderSignature {
        bytes32 config;
        bytes32 r;
        bytes32 s;
    }

code/contracts/lib/LibEIP712.sol:L3-L10

library LibEIP712 {
    string internal constant DOMAIN_NAME = "Mai Protocol";

    struct OrderSignature {
        bytes32 config;
        bytes32 r;
        bytes32 s;
    }

Description

UPDATE: The client provided the following statement: Fixed. NOTE: The assessment team would like to note that function visibility could still be refined (e.g. ContractReader, ChainlinkAdapter, Exchange, *Governance, …)

Review function attributes of functions that are never called from the current contract. These methods can be declared as external instead of public in order to safe gas and make the reader aware, that the method is only called by an external entity.

For example, public methods in contractReader, PerpetualProxy, GlobalConfig, Exchange, Governance functionality in Perpetual, AMM and other exposed API in the contract system may be declared external.

Description

Pre-compute static hashed values that are known at compile-time to save some gas and add a comment describing the hashed value.

code/contracts/lib/LibEIP712.sol:L15-L16

bytes32 private constant EIP712_DOMAIN_TYPEHASH = keccak256(abi.encodePacked("EIP712Domain(string name)"));

Description

The exchange provides means for the trader or broker to cancel the order. The cancelOrder method, however, only stores the hash of the canceled order in mapping but the mapping is never checked. It is therefore effectively impossible for a trader to cancel an order.

Examples

code/contracts/exchange/Exchange.sol:L179-L187

function cancelOrder(LibOrder.Order memory order) public {
    require(msg.sender == order.trader || msg.sender == order.broker, "invalid caller");

    bytes32 orderHash = order.getOrderHash();
    cancelled[orderHash] = true;

    emit Cancel(orderHash);
}

Recommendation

• matchOrders* or validateOrderParam should check if cancelled[orderHash] == true and abort fulfilling the order.
• Verify the order params (Signature) before accepting it as canceled.

Description

The specification for AMM.funding() states isEmergency==FALSE as a requirement. However, the state isEmergency does not exist (we assume EMERGENCY aka. SETTLING) and the implementation does not perform any state checks. This method is called by many other functions in AMM.

Recommendation

According to the specification, forceFunding should not be allowed in EMERGENCY mode. However, it is assumed that this method should only be callable in NORMAL mode.

The assessment team would like to note that the specification appears to be inconsistent and dated (method names, variable names, …).

Description

According to the specification withdraw can only be called in NORMAL state. However, the implementation allows it to be called in NORMAL and SETTLED mode.

Examples

Withdraw only checks for !SETTLING state which resolves to NORMAL and SETTLED.

code/contracts/perpetual/Perpetual.sol:L175-L178

function withdraw(uint256 amount) public {
    withdrawFromAccount(msg.sender, amount);
}

code/contracts/perpetual/Perpetual.sol:L156-L169

function withdrawFromAccount(address payable guy, uint256 amount) private {
    require(guy != address(0), "invalid guy");
    require(status != LibTypes.Status.SETTLING, "wrong perpetual status");

    uint256 currentMarkPrice = markPrice();
    require(isSafeWithPrice(guy, currentMarkPrice), "unsafe before withdraw");
    remargin(guy, currentMarkPrice);
    address broker = currentBroker(guy);
    bool forced = broker == address(amm.perpetualProxy()) || broker == address(0);
    withdraw(guy, amount, forced);

    require(isSafeWithPrice(guy, currentMarkPrice), "unsafe after withdraw");
    require(availableMarginWithPrice(guy, currentMarkPrice) >= 0, "withdraw margin");
}

In contrast, withdrawFor requires the state to be NORMAL:

code/contracts/perpetual/Perpetual.sol:L171-L174

function withdrawFor(address payable guy, uint256 amount) public onlyWhitelisted {
    require(status == LibTypes.Status.NORMAL, "wrong perpetual status");
    withdrawFromAccount(guy, amount);
}

Recommendation

withdraw should only be available in the NORMAL operation mode.

Description

withdrawFromInsurance checks that enough funds are in the insurance fund before allowing withdrawal by an admin by checking the provided rawAmount <= insuranceFundBalance.toUint256(). rawAmount is the ETH (18 digit precision) or collateral token amount (can be less than 18 digit precision) to be withdrawn while insuranceFundBalance is a WAD-denominated value (18 digit precision).

The check does not hold if the configured collateral has different precision and may have unwanted consequences, e.g. the withdrawal of more funds than expected.

Note: there is another check for insuranceFundBalance staying positive after the potential external call to collateral.

Examples

code/contracts/perpetual/Perpetual.sol:L204-L216

function withdrawFromInsuranceFund(uint256 rawAmount) public onlyWhitelistAdmin {
    require(rawAmount > 0, "invalid amount");
    require(insuranceFundBalance > 0, "insufficient funds");
    require(rawAmount <= insuranceFundBalance.toUint256(), "insufficient funds");

    int256 wadAmount = toWad(rawAmount);
    insuranceFundBalance = insuranceFundBalance.sub(wadAmount);
    withdrawFromProtocol(msg.sender, rawAmount);

    require(insuranceFundBalance >= 0, "negtive insurance fund");

    emit UpdateInsuranceFund(insuranceFundBalance);
}

When looking at the test-cases there seems to be a misconception about what unit of amount withdrawFromInsuranceFund is taking. For example, the insurance fund withdrawal and deposit are not tested for collateral that specifies a precision that is not 18. The test-cases falsely assume that the input to withdrawFromInsuranceFund is a WAD value, while it is taking the collateral’s rawAmount which is then converted to a WAD number.

code/test/test_perpetual.js:L471-L473

await perpetual.withdrawFromInsuranceFund(toWad(10.111));
fund = await perpetual.insuranceFundBalance();
assert.equal(fund.toString(), 0);

Recommendation

Check that require(wadAmount <= insuranceFundBalance.toUint256(), "insufficient funds");, add a test-suite testing the insurance fund with collaterals with different precision and update existing tests that properly provide the expected input to withdraFromInsurance.

Description

Perpetual.liquidate is used to liquidate an account that is “unsafe,” determined by the relative sizes of marginBalanceWithPrice and maintenanceMarginWithPrice:

code/contracts/perpetual/Perpetual.sol:L248-L253

// safe for liquidation
function isSafeWithPrice(address guy, uint256 currentMarkPrice) public returns (bool) {
    return
        marginBalanceWithPrice(guy, currentMarkPrice) >=
        maintenanceMarginWithPrice(guy, currentMarkPrice).toInt256();
}

Perpetual.liquidate allows the caller to assume the liquidated account’s position, as well as a small amount of “penalty collateral.” The steps to liquidate are, roughly:

1. Close the liquidated account’s position
2. Perform a trade on the liquidated assets with the liquidator acting as counter-party
3. Grant the liquidator a portion of the liquidated assets as a reward. An additional portion is added to the insurance fund.
4. Handle any losses

We found several issues in Perpetual.liquidate:

Examples

liquidateFrom has public visibility:

code/contracts/perpetual/Perpetual.sol:L270

function liquidateFrom(address from, address guy, uint256 maxAmount) public returns (uint256, uint256) {

Given that liquidate only calls liquidateFrom after checking the current contract’s status, this oversight allows anyone to call liquidateFrom during the SETTLED stage:

code/contracts/perpetual/Perpetual.sol:L291-L294

function liquidate(address guy, uint256 maxAmount) public returns (uint256, uint256) {
    require(status != LibTypes.Status.SETTLED, "wrong perpetual status");
    return liquidateFrom(msg.sender, guy, maxAmount);
}

Additionally, directly calling liquidateFrom allows anyone to liquidate on behalf of other users, forcing other accounts to assume liquidated positions.

Finally, neither liquidate nor liquidateFrom check that the liquidated account and liquidator are the same. Though the liquidation accounting process is hard to follow, we believe this is unintended and could lead to large errors in internal contract accounting.

Recommendation

• Make liquidateFrom an internal function

• In liquidate or liquidateFrom, check that msg.sender != guy

Description

In a number of cases, administrators of contracts can update or upgrade things in the system without warning. This has the potential to violate a security goal of the system.

Specifically, privileged roles could use front running to make malicious changes just ahead of incoming transactions, or purely accidental negative effects could occur due to unfortunate timing of changes.

Some instances of this are more important than others, but in general users of the system should have assurances about the behavior of the action they’re about to take.

Examples

The deployer of the PerpetualGovernance, AMMGovernance, and GlobalConfig contracts are set as administrators for the contracts through WhitelistedRole. The WhitelistedAdminRole can whitelist other accounts at any time and allow them to perform actions protected by the onlyWhitelisted decorator.

Updating governance and global configuration parameters are not protected by a time-lock and take effect immediately. This, therefore, creates an opportunity for administrators to front-run users on the exchange by changing parameters for orders. It may also allow an administrator to temporarily lift restrictions for themselves (e.g. withdrawalLockBlockCount).

• GlobalConfig
  • withdrawalLockBlockCount is queried when applying for withdrawal. This value can be set zero enabling allowing immediate withdrawal.
  • brokerLockBlockCount is queried when setting a new broker. This value can e set to zero effectively enabling immediate broker changes.

code/contracts/global/GlobalConfig.sol:L18-L27

function setGlobalParameter(bytes32 key, uint256 value) public onlyWhitelistAdmin {
    if (key == "withdrawalLockBlockCount") {
        withdrawalLockBlockCount = value;
    } else if (key == "brokerLockBlockCount") {
        brokerLockBlockCount = value;
    } else {
        revert("key not exists");
    }
    emit UpdateGlobalParameter(key, value);
}

• PerpetualGovernance
  • e.g. Admin can front-run specific matchOrder calls and set arbitrary dev fees or curve parameters…

code/contracts/perpetual/PerpetualGovernance.sol:L39-L80

function setGovernanceParameter(bytes32 key, int256 value) public onlyWhitelistAdmin {
    if (key == "initialMarginRate") {
        governance.initialMarginRate = value.toUint256();
        require(governance.initialMarginRate > 0, "require im > 0");
        require(governance.initialMarginRate < 10**18, "require im < 1");
        require(governance.maintenanceMarginRate < governance.initialMarginRate, "require mm < im");
    } else if (key == "maintenanceMarginRate") {
        governance.maintenanceMarginRate = value.toUint256();
        require(governance.maintenanceMarginRate > 0, "require mm > 0");
        require(governance.maintenanceMarginRate < governance.initialMarginRate, "require mm < im");
        require(governance.liquidationPenaltyRate < governance.maintenanceMarginRate, "require lpr < mm");
        require(governance.penaltyFundRate < governance.maintenanceMarginRate, "require pfr < mm");
    } else if (key == "liquidationPenaltyRate") {
        governance.liquidationPenaltyRate = value.toUint256();
        require(governance.liquidationPenaltyRate < governance.maintenanceMarginRate, "require lpr < mm");
    } else if (key == "penaltyFundRate") {
        governance.penaltyFundRate = value.toUint256();
        require(governance.penaltyFundRate < governance.maintenanceMarginRate, "require pfr < mm");
    } else if (key == "takerDevFeeRate") {
        governance.takerDevFeeRate = value;
    } else if (key == "makerDevFeeRate") {
        governance.makerDevFeeRate = value;
    } else if (key == "lotSize") {
        require(
            governance.tradingLotSize == 0 || governance.tradingLotSize.mod(value.toUint256()) == 0,
            "require tls % ls == 0"
        );
        governance.lotSize = value.toUint256();
    } else if (key == "tradingLotSize") {
        require(governance.lotSize == 0 || value.toUint256().mod(governance.lotSize) == 0, "require tls % ls == 0");
        governance.tradingLotSize = value.toUint256();
    } else if (key == "longSocialLossPerContracts") {
        require(status == LibTypes.Status.SETTLING, "wrong perpetual status");
        socialLossPerContracts[uint256(LibTypes.Side.LONG)] = value;
    } else if (key == "shortSocialLossPerContracts") {
        require(status == LibTypes.Status.SETTLING, "wrong perpetual status");
        socialLossPerContracts[uint256(LibTypes.Side.SHORT)] = value;
    } else {
        revert("key not exists");
    }
    emit UpdateGovernanceParameter(key, value);
}

• Admin can set devAddress or even update to a new amm and globalConfig

code/contracts/perpetual/PerpetualGovernance.sol:L82-L94

function setGovernanceAddress(bytes32 key, address value) public onlyWhitelistAdmin {
    require(value != address(0x0), "invalid address");
    if (key == "dev") {
        devAddress = value;
    } else if (key == "amm") {
        amm = IAMM(value);
    } else if (key == "globalConfig") {
        globalConfig = IGlobalConfig(value);
    } else {
        revert("key not exists");
    }
    emit UpdateGovernanceAddress(key, value);
}

• AMMGovernance

code/contracts/liquidity/AMMGovernance.sol:L22-L43

function setGovernanceParameter(bytes32 key, int256 value) public onlyWhitelistAdmin {
    if (key == "poolFeeRate") {
        governance.poolFeeRate = value.toUint256();
    } else if (key == "poolDevFeeRate") {
        governance.poolDevFeeRate = value.toUint256();
    } else if (key == "emaAlpha") {
        require(value > 0, "alpha should be > 0");
        governance.emaAlpha = value;
        emaAlpha2 = 10**18 - governance.emaAlpha;
        emaAlpha2Ln = emaAlpha2.wln();
    } else if (key == "updatePremiumPrize") {
        governance.updatePremiumPrize = value.toUint256();
    } else if (key == "markPremiumLimit") {
        governance.markPremiumLimit = value;
    } else if (key == "fundingDampener") {
        governance.fundingDampener = value;
    } else {
        revert("key not exists");
    }
    emit UpdateGovernanceParameter(key, value);
}

Recommendation

The underlying issue is that users of the system can’t be sure what the behavior of a function call will be, and this is because the behavior can change at any time.

We recommend giving the user advance notice of changes with a time lock. For example, make all updates to system parameters or upgrades require two steps with a mandatory time window between them. The first step merely broadcasts to users that a particular change is coming, and the second step commits that change after a suitable waiting period.

Additionally, users should verify the whitelist setup before using the contract system and monitor it for new additions to the whitelist. Documentation should clearly outline what roles are owned by whom to support suitability. Sane parameter bounds should be enforced (e.g. min. disallow block delay of zero )

Description

According to https://en.wikipedia.org/wiki/Moving_average

The coefficient α represents the degree of weighting decrease, a constant smoothing factor between 0 and 1. A higher α discounts older observations faster.

However, the code does not check upper bounds. An admin may, therefore, set an invalid alpha that puts emaAlpha2 out of bounds or negative.

Examples

code/contracts/liquidity/AMMGovernance.sol:L27-L31

} else if (key == "emaAlpha") {
    require(value > 0, "alpha should be > 0");
    governance.emaAlpha = value;
    emaAlpha2 = 10**18 - governance.emaAlpha;
    emaAlpha2Ln = emaAlpha2.wln();

Recommendation

Ensure that the system configuration is always within safe bounds. Document expected system variable types and their safe operating ranges. Enforce that bounds are checked every time a value is set. Enforce safe defaults when deploying contracts.

Ensure emaAlpha is 0 < value < 1 WAD

Description

When providing liquidity with addLiquidity(), the amount of collateral required is based on the current price and the amount of shares received depends on the total amount of shares in circulation. This price can fluctuate at a moment’s notice, making the behavior of the function unpredictable for the user.

The same is true when removing liquidity via removeLiquidity().

Recommendation

Unpredictability can be introduced by someone front-running the transaction, or simply by poor timing. For example, adjustments to global variable configuration by the system admin will directly impact subsequent actions by the user. In order to ensure users know what to expect:

• Allow the caller to specify a price limit or maximum amount of collateral to be spent

• Allow the caller to specify the minimum amount of shares expected to be received

Description

matchOrders does not check that that the sender has provided the same number of amounts as makerOrderParams. When fewer amounts exist than makerOrderParams, the method will revert because of an out-of-bounds array access. When fewer makerOrderParams exist than amounts, the method will succeed, and the additional values in amounts will be ignored.

Additionally, the method allows the sender to provide no makerOrderParams at all, resulting in no state changes.

matchOrders also does not reject trades with an amount set to zero. Such orders should be rejected because they do not comply with the minimum tradingLotSize configured for the system. As a side-effect, events may be emitted for zero-amount trades and unexpected state changes may occur.

Examples

code/contracts/exchange/Exchange.sol:L34-L39

function matchOrders(
    LibOrder.OrderParam memory takerOrderParam,
    LibOrder.OrderParam[] memory makerOrderParams,
    address _perpetual,
    uint256[] memory amounts
) public {

code/contracts/exchange/Exchange.sol:L113-L113

function matchOrderWithAMM(LibOrder.OrderParam memory takerOrderParam, address _perpetual, uint256 amount) public {

Recommendation

• Require makerOrderParams.length > 0 && amounts.length == makerOrderParams.length
• Require that amount or any of the amounts[i] provided to matchOrders is >=tradingLotSize.

Description

When removing liquidity, the amount of collateral received is calculated from the shareAmount (ShareToken) of the liquidity provider. The liquidity removal process registers a trade on the amount, with the liquidity provider and AMM taking opposite sides. Because trading only accepts multiple of the lotSize, the leftover is discarded. The amount discarded may be up to lotSize - 1.

The expectation is that this value should not be too high, but as lotSize can be set to arbitrary values by an admin, it is possible that this step discards significant value. Additionally, see issue 6.6 for how this can be exploited by an admin.

Note that similar behavior is present in Perpetual.liquidateFrom, where the liquidatableAmount calculated undergoes a similar modulo operation:

code/contracts/perpetual/Perpetual.sol:L277-L278

uint256 liquidatableAmount = totalPositionSize.sub(totalPositionSize.mod(governance.lotSize));
liquidationAmount = liquidationAmount.ceil(governance.lotSize).min(maxAmount).min(liquidatableAmount);

Examples

• lotSize can arbitrarily be set up to pos_int256_max as long as tradingLotSize % lotSize == 0

code/contracts/perpetual/PerpetualGovernance.sol:L61-L69

} else if (key == "lotSize") {
    require(
        governance.tradingLotSize == 0 || governance.tradingLotSize.mod(value.toUint256()) == 0,
        "require tls % ls == 0"
    );
    governance.lotSize = value.toUint256();
} else if (key == "tradingLotSize") {
    require(governance.lotSize == 0 || value.toUint256().mod(governance.lotSize) == 0, "require tls % ls == 0");
    governance.tradingLotSize = value.toUint256();

• amount is derived from shareAmount rounded down to the next multiple of the lotSize. The leftover is discarded.

code/contracts/liquidity/AMM.sol:L289-L294

uint256 amount = shareAmount.wmul(oldPoolPositionSize).wdiv(shareToken.totalSupply());
amount = amount.sub(amount.mod(perpetualProxy.lotSize()));

perpetualProxy.transferBalanceOut(trader, price.wmul(amount).mul(2));
burnShareTokenFrom(trader, shareAmount);
uint256 opened = perpetualProxy.trade(trader, LibTypes.Side.LONG, price, amount);

Recommendation

• Ensure that documentation makes users aware of the fact that they may lose up to lotsize-1 in value.

• Alternatively, track accrued value and permit trades on values that exceed lotSize. Note that this may add significant complexity.

• Ensure that similar system behavior, like the liquidatableAmount calculated in Perpetual.liquidateFrom, is also documented and communicated clearly to users.

Description

The external Chainlink oracle, which provides index price information to the system, introduces risk inherent to any dependency on third-party data sources. For example, the oracle could fall behind or otherwise fail to be maintained, resulting in outdated data being fed to the index price calculations of the AMM. Oracle reliance has historically resulted in crippled on-chain systems, and complications that lead to these outcomes can arise from things as simple as network congestion.

Ensuring that unexpected oracle return values are properly handled will reduce reliance on off-chain components and increase the resiliency of the smart contract system that depends on them.

Examples

1. The ChainlinkAdapter and InversedChainlinkAdapter take the oracle’s (int256) latestAnswer and convert the result using chainlinkDecimalsAdapter. This arithmetic operation can underflow/overflow if the Oracle provides a large enough answer:

code/contracts/oracle/ChainlinkAdapter.sol:L10-L19

int256 public constant chainlinkDecimalsAdapter = 10**10;

constructor(address _feeder) public {
    feeder = IChainlinkFeeder(_feeder);
}

function price() public view returns (uint256 newPrice, uint256 timestamp) {
    newPrice = (feeder.latestAnswer() * chainlinkDecimalsAdapter).toUint256();
    timestamp = feeder.latestTimestamp();
}

code/contracts/oracle/InversedChainlinkAdapter.sol:L11-L20

int256 public constant chainlinkDecimalsAdapter = 10**10;

constructor(address _feeder) public {
    feeder = IChainlinkFeeder(_feeder);
}

function price() public view returns (uint256 newPrice, uint256 timestamp) {
    newPrice = ONE.wdiv(feeder.latestAnswer() * chainlinkDecimalsAdapter).toUint256();
    timestamp = feeder.latestTimestamp();
}

2. The oracle provides a timestamp for the latestAnswer that is not validated and may lead to old oracle timestamps being accepted (e.g. caused by congestion on the blockchain or a directed censorship attack).

code/contracts/oracle/InversedChainlinkAdapter.sol:L19-L20

    timestamp = feeder.latestTimestamp();
}

Recommendation

• Use SafeMath for mathematical computations

• Verify latestAnswer is within valid bounds (!=0)

• Verify latestTimestamp is within accepted bounds (not in the future, was updated within a reasonable amount of time)

• Deduplicate code by combining both Adapters into one as the only difference is that the InversedChainlinkAdapter returns ONE.wdiv(price).

Description

createPool can be initialized with amount == 0. Because a subsequent call to initFunding can only happen once, the contract is now initialized with a zero size pool that does not allow any liquidity to be added.

Trying to recover by calling createPool again fails as the funding state is already initialized. The specification also states the following about createPool:

Open asset pool by deposit to AMM. Only available when pool is empty.

This is inaccurate, as createPool can only be called once due to a check in initFunding, but this call may leave the pool empty.

Furthermore, the contract’s liquidity management functionality (addLiquidity and removeLiquidity) allows adding zero liquidity (amount == 0) and removing zero shares (shareAmount == 0). As these actions do not change the liquidity of the pool, they should be rejected.

Recommendation

• Require a minimum amount lotSize to be provided when creating a Pool and adding liquidity via addLiquidity

• Require a minimum amount of shares to be provided when removing liquidity via removeLiquidity

Description

There is no limitation on how long an administrator can put the Perpetual contract into emergency mode. Users cannot trade or withdraw funds in emergency mode and are effectively locked out until the admin chooses to put the contract in SETTLED mode.

Examples

code/contracts/perpetual/PerpetualGovernance.sol:L96-L101

function beginGlobalSettlement(uint256 price) public onlyWhitelistAdmin {
    require(status != LibTypes.Status.SETTLED, "already settled");
    settlementPrice = price;
    status = LibTypes.Status.SETTLING;
    emit BeginGlobalSettlement(price);
}

code/contracts/perpetual/Perpetual.sol:L146-L154

function endGlobalSettlement() public onlyWhitelistAdmin {
    require(status == LibTypes.Status.SETTLING, "wrong perpetual status");

    address guy = address(amm.perpetualProxy());
    settleFor(guy);
    status = LibTypes.Status.SETTLED;

    emit EndGlobalSettlement();
}

Recommendation

• Set a time-lock when entering emergency mode that allows anyone to set the system to SETTLED after a fixed amount of time.

Description

Signed order data may be re-usable cross-chain as the chain-id is not explicitly part of the signed data.

It is also recommended to further harden the signature verification and validate that v and s are within expected bounds. ecrecover() returns 0x0 to indicate an error condition, therefore, a signerAddress or recovered address of 0x0 should explicitly be disallowed.

Examples

The signed order data currently includes the EIP712 Domain Name Mai Protocol and the following information:

code/contracts/lib/LibOrder.sol:L23-L48

struct Order {
    address trader;
    address broker;
    address perpetual;
    uint256 amount;
    uint256 price;
    /**
     * Data contains the following values packed into 32 bytes
     * ╔════════════════════╤═══════════════════════════════════════════════════════════╗
     * ║                    │ length(bytes)   desc                                      ║
     * ╟────────────────────┼───────────────────────────────────────────────────────────╢
     * ║ version            │ 1               order version                             ║
     * ║ side               │ 1               0: buy (long), 1: sell (short)            ║
     * ║ isMarketOrder      │ 1               0: limitOrder, 1: marketOrder             ║
     * ║ expiredAt          │ 5               order expiration time in seconds          ║
     * ║ asMakerFeeRate     │ 2               maker fee rate (base 100,000)             ║
     * ║ asTakerFeeRate     │ 2               taker fee rate (base 100,000)             ║
     * ║ (d) makerRebateRate│ 2               rebate rate for maker (base 100)          ║
     * ║ salt               │ 8               salt                                      ║
     * ║ isMakerOnly        │ 1               is maker only                             ║
     * ║ isInversed         │ 1               is inversed contract                      ║
     * ║                    │ 8               reserved                                  ║
     * ╚════════════════════╧═══════════════════════════════════════════════════════════╝
     */
    bytes32 data;
}

Signature verification:

code/contracts/lib/LibSignature.sol:L24-L47

function isValidSignature(OrderSignature memory signature, bytes32 hash, address signerAddress)
    internal
    pure
    returns (bool)
{
    uint8 method = uint8(signature.config[1]);
    address recovered;
    uint8 v = uint8(signature.config[0]);

    if (method == uint8(SignatureMethod.ETH_SIGN)) {
        recovered = ecrecover(
            keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", hash)),
            v,
            signature.r,
            signature.s
        );
    } else if (method == uint8(SignatureMethod.EIP712)) {
        recovered = ecrecover(hash, v, signature.r, signature.s);
    } else {
        revert("invalid sign method");
    }

    return signerAddress == recovered;
}

Recommendation

• Include the chain-id in the signature to avoid cross-chain validity of signatures
• verify s is within valid bounds to avoid signature malleability

if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
      revert("ECDSA: invalid signature 's' value");
 }

• verify v is within valid bounds

if (v != 27 && v != 28) {
     revert("ECDSA: invalid signature 'v' value");
}

• return invalid if the result of ecrecover() is 0x0

Description

validateOrderParam verifies the signature and version of a provided order. Instead of checking against the contract constant SUPPORTED_ORDER_VERSION it, however, checks against a hardcoded version 2 in the method itself.

This might be a problem if SUPPORTED_ORDER_VERSION is seen as the configuration parameter for the allowed version. Changing it would not change the allowed order version for validateOrderParam as this constant literal is never used.

At the time of this audit, however, the SUPPORTED_ORDER_VERSION value equals the hardcoded value in the validateOrderParam method.

Examples

code/contracts/exchange/Exchange.sol:L155-L170

function validateOrderParam(IPerpetual perpetual, LibOrder.OrderParam memory orderParam)
    internal
    view
    returns (bytes32)
{
    address broker = perpetual.currentBroker(orderParam.trader);
    require(broker == msg.sender, "invalid broker");
    require(orderParam.getOrderVersion() == 2, "unsupported version");
    require(orderParam.getExpiredAt() >= block.timestamp, "order expired");

    bytes32 orderHash = orderParam.getOrderHash(address(perpetual), broker);
    require(orderParam.signature.isValidSignature(orderHash, orderParam.trader), "invalid signature");
    require(filled[orderHash] < orderParam.amount, "fullfilled order");

    return orderHash;
}

Recommendation

Check against SUPPORTED_ORDER_VERSION instead of the hardcoded value 2.

Description

LibMathSigned.wpowi(x,n) calculates Wad value x (base) to the power of n (exponent). The exponent is declared as a signed int, however, the method returns wrong results when calculating x ^(-n).

The comment for the wpowi method suggests that n is a normal integer instead of a Wad-denominated value. This, however, is not being enforced.

Examples

• LibMathSigned.wpowi(8000000000000000000, 2) = 64000000000000000000
• (wrong) LibMathSigned.wpowi(8000000000000000000, -2) = 64000000000000000000

code/contracts/lib/LibMath.sol:L103-L116

// x ^ n
// NOTE: n is a normal integer, do not shift 18 decimals
// solium-disable-next-line security/no-assign-params
function wpowi(int256 x, int256 n) internal pure returns (int256 z) {
    z = n % 2 != 0 ? x : _WAD;

    for (n /= 2; n != 0; n /= 2) {
        x = wmul(x, x);

        if (n % 2 != 0) {
            z = wmul(z, x);
        }
    }
}

Recommendation

Make wpowi support negative exponents or use the proper type for n (uint) and reject negative values.

Enforce that the exponent bounds are within sane ranges and less than a Wad to detect potential misuse where someone accidentally provides a Wad value as n.

Add positive and negative unit-tests to fully cover this functionality.

Description

Using an outdated compiler version can be problematic especially if there are publicly disclosed bugs and issues (see also https://github.com/ethereum/solidity/releases) that affect the current compiler version.

The codebase specifies a floating version of ^0.5.2 and makes use of the experimental feature ABIEncoderV2.

It should be noted, that ABIEncoderV2 was subject to multiple bug-fixes up until the latest 0.6.xversion and contracts compiled with earlier versions are - for example - susceptible to the following issues:

• ImplicitConstructorCallvalueCheck
• TupleAssignmentMultiStackSlotComponents
• MemoryArrayCreationOverflow
• privateCanBeOverridden
• YulOptimizerRedundantAssignmentBreakContinue0.5
• ABIEncoderV2CalldataStructsWithStaticallySizedAndDynamicallyEncodedMembers
• SignedArrayStorageCopy
• ABIEncoderV2StorageArrayWithMultiSlotElement
• DynamicConstructorArgumentsClippedABIV2

Examples

Codebase declares compiler version ^0.5.2:

code/contracts/liquidity/AMM.sol:L1-L2

pragma solidity ^0.5.2;
pragma experimental ABIEncoderV2; // to enable structure-type parameters

According to etherscan.io, the currently deployed main-net AMM contract is compiled with solidity version 0.5.8:

https://etherscan.io/address/0xb95B9fb0539Ec84DeD2855Ed1C9C686Af9A4e8b3#code

Recommendation

It is recommended to settle on the latest stable 0.6.x or 0.5.x version of the Solidity compiler and lock the pragma version to a specifically tested compiler release.

Description

The const ONE_WAD_U is declared but never used. Avoid re-declaring the same constants in multiple source-units (and unit-test cases) as this will be hard to maintain.

Examples

code/contracts/liquidity/AMM.sol:L17-L17

uint256 private constant ONE_WAD_U = 10**18;

Recommendation

Remove unused code. Import the value from a shared resource. E.g.ONE_WAD is declared multiple times in LibMathSigned, LibMathUnsigned, AMM, hardcoded in checks in PerpetualGovernance.setGovernanceParameter, AMMGovernance.setGovernanceParameter.

Description

Perpetual inherits from PerpetualGovernance and Collateral, which declare state variables that are shadowed in the Perpetual constructor.

Examples

• Local constructor argument shadows PerpetualGovernance.globalConfig, PerpetualGovernance.devAddress, Collateral.collateral

Note: Confusing name: Collateral is an inherited contract and a state variable.

code/contracts/perpetual/Perpetual.sol:L34-L41

constructor(address globalConfig, address devAddress, address collateral, uint256 collateralDecimals)
    public
    Position(collateral, collateralDecimals)
{
    setGovernanceAddress("globalConfig", globalConfig);
    setGovernanceAddress("dev", devAddress);
    emit CreatePerpetual();
}

Recommendation

Rename the parameter or state variable.

Description

When initializing the Perpetual contract, the deployer can decide to use either ETH, or an ERC20-compliant collateral. In the latter case, the deployer must provide a nonzero address for the token, as well as the number of decimals used by the token:

code/contracts/perpetual/Collateral.sol:L28-L34

constructor(address _collateral, uint256 decimals) public {
    require(decimals <= MAX_DECIMALS, "decimals out of range");
    require(_collateral != address(0x0) || (_collateral == address(0x0) && decimals == 18), "invalid decimals");

    collateral = _collateral;
    scaler = (decimals == MAX_DECIMALS ? 1 : 10**(MAX_DECIMALS - decimals)).toInt256();
}

The provided decimals value is not checked for validity and can differ from the actual token’s decimals.

Recommendation

Ensure to establish documentation that makes users aware of the fact that the decimals configured are not enforced to match the actual tokens decimals. This is to allow users to audit the system configuration and decide whether they want to participate in it.

Description

ShareToken is an extension of the Openzeppelin ERC20Mintable pattern which exposes a method called mint() that allows accounts owning the minter role to mint new tokens. The return value of ShareToken.mint() is not checked.

Since the ERC20 standard does not define whether this method should return a value or revert it may be problematic to assume that all tokens revert. If, for example, an implementation is used that does not revert on error but returns a boolean error indicator instead the caller might falsely continue without the token minted.

We would like to note that the functionality is intended to be used with the provided ShareToken and therefore the contract is safe to use assuming ERC20Mintable.mint reverts on error. The issue arises if the system is used with a different ShareToken implementation that is not implemented in the same way.

Examples

• Openzeppelin implementation

function mint(address account, uint256 amount) public onlyMinter returns (bool) {
    _mint(account, amount);
    return true;
}

• Call with unchecked return value

code/contracts/liquidity/AMM.sol:L499-L502

function mintShareTokenTo(address guy, uint256 amount) internal {
    shareToken.mint(guy, amount);
}

Recommendation

Consider wrapping the mint statement in a require clause, however, this way only tokens that are returning a boolean error indicator are supported. Document the specification requirements for the ShareToken and clearly state if the token is expected to revert or return an error indicator.

It should also be documented that the Token exposes a burn method that does not adhere to the Openzeppelin ERC20Burnable implementation. The ERC20Burnable import is unused as noted in issue 6.23.

Description

The system can be put into emergency mode by an admin calling beginGlobalSettlement and providing a fixed settlementPrice. The method can be invoked even when the contract is already in SETTLING (emergency) mode, allowing an admin to selectively adjust the settlement price again. This does not seem to be the intended behavior as calling the method again re-sets the status to SETTLING. Furthermore, it may affect users’ behavior during the SETTLING phase.

Examples

code/contracts/perpetual/PerpetualGovernance.sol:L96-L101

function beginGlobalSettlement(uint256 price) public onlyWhitelistAdmin {
    require(status != LibTypes.Status.SETTLED, "already settled");
    settlementPrice = price;
    status = LibTypes.Status.SETTLING;
    emit BeginGlobalSettlement(price);
}

Recommendation

• Emergency mode should only be allowed to be set once

Description

The following source units are imported but not referenced in the contract:

Examples

code/contracts/perpetual/Perpetual.sol:L4-L5

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/SafeERC20.sol";

code/contracts/perpetual/Perpetual.sol:L14-L15

import "../interface/IPriceFeeder.sol";
import "../interface/IGlobalConfig.sol";

code/contracts/token/ShareToken.sol:L5-L5

import "@openzeppelin/contracts/token/ERC20/ERC20Burnable.sol";

code/contracts/token/ShareToken.sol:L3-L3

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

Recommendation

Check all imports and remove all unused/unreferenced and unnecessary imports.

Description

The enum OrderStatus is declared but never used.

Examples

code/contracts/exchange/Exchange.sol:L20-L20

enum OrderStatus {EXPIRED, CANCELLED, FILLABLE, FULLY_FILLED}

Recommendation

Remove unused code.

Description

LibMathUnsigned declares _UINT256_MAX as 2^255-1 while this value actually represents _INT256_MAX. This appears to just be a naming issue.

Examples

(UINT256_MAX/2-1 => pos INT256_MAX; 2**256/2-1==2**255-1)

code/contracts/lib/LibMath.sol:L228-L230

library LibMathUnsigned {
    uint256 private constant _WAD = 10**18;
    uint256 private constant _UINT256_MAX = 2**255 - 1;

Recommendation

Rename _UINT256_MAX to _INT256MAX or _SIGNED_INT256MAX.

Description

The assertion below states that logE only accepts v <= 1e22 * 1e18 while the argument name is x. In addition to that we suggest representing large literals in scientific notation.

Examples

code/contracts/lib/LibMath.sol:L153-L157

function wln(int256 x) internal pure returns (int256) {
    require(x > 0, "logE of negative number");
    require(x <= 10000000000000000000000000000000000000000, "logE only accepts v <= 1e22 * 1e18"); // in order to prevent using safe-math
    int256 r = 0;
    uint8 extra_digits = longer_digits - fixed_digits;

Recommendation

Update the inconsistent assertion text v -> x and represent large literals in scientific notation as they are otherwise difficult to read and review.

Description

The method LibMathSigned.roundHalfUp(int x, int y) returns the value x rounded up to the base y. The method suggests that the result is the rounded value while that’s not actually true. The result for a positive x is x + base/2 and x - base/2 for negative values. The rounding is not yet finished as this would require a final division by base y to manifest the rounding.

It is assumed that the final rounding step is not executed for performance reasons. However, this might easily introduce errors when the caller assumes the result is rounded for base while it is not.

Examples

• roundHalfUp(-4700, 1000) = -4700 instead of 5000
• roundHalfUp(4700, 1000) = 4700 instead of 5000

code/contracts/lib/LibMath.sol:L126-L133

// ROUND_HALF_UP rule helper. 0.5 ≈ 1, 0.4 ≈ 0, -0.5 ≈ -1, -0.4 ≈ 0
function roundHalfUp(int256 x, int256 y) internal pure returns (int256) {
    require(y > 0, "roundHalfUp only supports y > 0");
    if (x >= 0) {
        return add(x, y / 2);
    }
    return sub(x, y / 2);
}

Recommendation

We have verified the current code-base and the callers for roundHalfUp are correctly finishing the rounding step. However, it is recommended to finish the rounding within the method or document this behavior to prevent errors caused by code that falsely assumes that the returned value finished rounding.

Description

The following methods declare a named return value but explicitly return a value instead. The named return value is not used.

• LibMathSigned.min()
• LibMathSigned.max()
• LibMathUnsigned.min()
• LibMathUnsigned.max()
• LibOrder.getOrderHash()
• LibOrder.hashOrder()

Examples

code/contracts/lib/LibMath.sol:L90-L96

function min(int256 x, int256 y) internal pure returns (int256 z) {
    return x <= y ? x : y;
}

function max(int256 x, int256 y) internal pure returns (int256 z) {
    return x >= y ? x : y;
}

code/contracts/lib/LibMath.sol:L285-L292

function min(uint256 x, uint256 y) internal pure returns (uint256 z) {
    return x <= y ? x : y;
}

function max(uint256 x, uint256 y) internal pure returns (uint256 z) {
    return x >= y ? x : y;
}

code/contracts/lib/LibOrder.sol:L68-L71

function getOrderHash(Order memory order) internal pure returns (bytes32 orderHash) {
    orderHash = LibEIP712.hashEIP712Message(hashOrder(order));
    return orderHash;
}

code/contracts/lib/LibOrder.sol:L86-L97

function hashOrder(Order memory order) internal pure returns (bytes32 result) {
    bytes32 orderType = EIP712_ORDER_TYPE;
    // solium-disable-next-line security/no-inline-assembly
    assembly {
        let start := sub(order, 32)
        let tmp := mload(start)
        mstore(start, orderType)
        result := keccak256(start, 224)
        mstore(start, tmp)
    }
    return result;
}

Recommendation

Remove the named return value and explicitly return the value.

Description

Rather than storing addresses and then casting to the known contract type, it’s better to use the best type available so the compiler can check for type safety.

Examples

Collateral. collateral is of type address, but it could be type IERC20 instead. Not only would this give a little more type safety when deploying new modules, but it would avoid repeated casts throughout the codebase of the form IERC20(collateral), IPerpetual(_perpetual) and others. The following is an incomplete list of examples:

• declare collateral as IERC20

code/contracts/perpetual/Collateral.sol:L19-L19

address public collateral;

code/contracts/perpetual/Collateral.sol:L51-L51

IERC20(collateral).safeTransferFrom(guy, address(this), rawAmount);

• declare argument perpetual as IPerpetual

code/contracts/exchange/Exchange.sol:L34-L42

function matchOrders(
    LibOrder.OrderParam memory takerOrderParam,
    LibOrder.OrderParam[] memory makerOrderParams,
    address _perpetual,
    uint256[] memory amounts
) public {
    require(!takerOrderParam.isMakerOnly(), "taker order is maker only");

    IPerpetual perpetual = IPerpetual(_perpetual);

• declare argument feeder as IChainlinkFeeder

code/contracts/oracle/ChainlinkAdapter.sol:L12-L14

constructor(address _feeder) public {
    feeder = IChainlinkFeeder(_feeder);
}

Remediation

Where possible, use more specific types instead of address. This goes for parameter types as well as state variable types.