Need an audit or want to learn more about Macro’s services?
Reach out for an audit or to learn more about Macro
Security Audit
July 21st, 2023
Version 1.0.0
Presented by 0xMacro
This document includes the results of the security audit for IDEX's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from March 28, 2023 to April 26, 2023.
The purpose of this audit is to review the source code of certain IDEX Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.
Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.
The following is an aggregation of issues found by the Macro Audit team:
| Severity | Count | Acknowledged | Won't Do | Addressed |
|---|---|---|---|---|
| Medium | 8 | - | 2 | 6 |
| Low | 4 | - | - | 4 |
| Code Quality | 5 | 2 | 1 | 2 |
| Informational | 2 | 1 | - | 1 |
IDEX was quick to respond to these issues.
Our understanding of the specification was based on the following sources:
IDEX’s v4 protocol is built upon a hybrid architecture, where the trading engine operates off-chain for allowing low-latency trades while providing on-chain order settlement and custody. Due to this hybrid design choice, a lot of functions are restricted and can only be accessed by privileged roles:
While the high number of restricted functions impose a high-level of centralization, users need to sign trades, transfers, and withdrawals so that no funds can be moved without the users permission (except for liquidating and depositing operations). In addition, users can always claim there funds in a permission-less way via the “wallet exit” mechanism. Note that users will usually receive less funds when closing the wallet via “wallet exit” as opposed to closing open positions via normal trades and then withdrawing the funds via IDEX’s exchange. This is needed to ensure the solvency of the protocol.
The following source code was reviewed during the audit:
94726de9d487e1e34fff62c2436bf23ebc9fbad8
Specifically, we audited the following contracts within this repository:
| Contract | SHA256 |
|---|---|
| contracts/Custodian.sol |
|
| contracts/Exchange.sol |
|
| contracts/Governance.sol |
|
| contracts/Owned.sol |
|
| contracts/libraries/AssetUnitConversions.sol |
|
| contracts/libraries/BalanceTracking.sol |
|
| contracts/libraries/ClosureDeleveraging.sol |
|
| contracts/libraries/Constants.sol |
|
| contracts/libraries/Depositing.sol |
|
| contracts/libraries/Enums.sol |
|
| contracts/libraries/ExitFund.sol |
|
| contracts/libraries/Funding.sol |
|
| contracts/libraries/FundingMultiplierQuartetHelper.sol |
|
| contracts/libraries/Hashing.sol |
|
| contracts/libraries/IndexPriceMargin.sol |
|
| contracts/libraries/Interfaces.sol |
|
| contracts/libraries/LiquidationValidations.sol |
|
| contracts/libraries/MarketAdmin.sol |
|
| contracts/libraries/MarketHelper.sol |
|
| contracts/libraries/Math.sol |
|
| contracts/libraries/NonceInvalidations.sol |
|
| contracts/libraries/OraclePriceMargin.sol |
|
| contracts/libraries/PositionBelowMinimumLiquidation.sol |
|
| contracts/libraries/PositionInDeactivatedMarketLiquidation.sol |
|
| contracts/libraries/SortedStringSet.sol |
|
| contracts/libraries/String.sol |
|
| contracts/libraries/Structs.sol |
|
| contracts/libraries/Time.sol |
|
| contracts/libraries/TradeValidations.sol |
|
| contracts/libraries/Trading.sol |
|
| contracts/libraries/Transferring.sol |
|
| contracts/libraries/UUID.sol |
|
| contracts/libraries/Validations.sol |
|
| contracts/libraries/WalletExitAcquisitionDeleveraging.sol |
|
| contracts/libraries/WalletExitLiquidation.sol |
|
| contracts/libraries/WalletExits.sol |
|
| contracts/libraries/WalletInMaintenanceAcquisitionDeleveraging.sol |
|
| contracts/libraries/WalletInMaintenanceLiquidation.sol |
|
| contracts/libraries/Withdrawing.sol |
|
| contracts/bridge-adapters/ExchangeStargateAdapter.sol |
|
Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.
Click on an issue to jump to it, or scroll down to see them all.
chainId in signature
withdrawExit from the IF wallet
safeTransfer/safeTransferFrom
withdrawNativeAsset
sgReceive can be called by anyone
We quantify issues in three parts:
This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:
| Severity | Description |
|---|---|
|
(C-x) Critical |
We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost. |
|
(H-x) High |
We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec. |
|
(M-x) Medium |
We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner. |
|
(L-x) Low |
The risk is small, unlikely, or may not relevant to the project in a meaningful way. Whether or not the project wants to develop a fix is up to the goals and needs of the project. |
|
(Q-x) Code Quality |
The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design. |
|
(I-x) Informational |
Warnings and things to keep in mind when operating the protocol. No immediate action required. |
|
(G-x) Gas Optimizations |
The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it. |
The deposit function in Exchange.sol does not account for tokens applying transfer tax, which results in less value transferred to the custodian than requested. However, the balance stored in BalanceTracking is using the full amount without considering the fees.
In the current version of IDEX v4, only USDC is supported as quote token. The USDC token contract implements upgradable logic, thus there is no guarantee that USDC will not add fee-on-transfer logic in the future.
Consider handling fee-on-transfer tokens correctly by calculating balanceAfter - balanceBefore when collateral is deposited to custodian.
The call to Chainlink’s price feed using latestRoundData() can return stale data in extreme situations such as highly volatile market conditions and flash crashes. In such situations, users could withdrawExit their wallet and benefit from outdated price data.
Consider checking that the returned updatedAt timestamp - as referenced in the Chainlink docs - falls within a valid time period.
withdrawExit does not validate oracle price update timing by design.
The protocol is not sensitive to stale oracle pricing on exit withdrawals. While exiting wallets may receive out-of-date pricing when liquidating positions, the protocol is not at risk of insolvency due to exit pricing.
Preventing exit withdrawals due to oracle price update timing validation introduces a source of withdrawal censorship.
The exchange uses hard-coded parameters listed in the GOVERNANCE readme.
Some of the parameters - defined in Constants.sol - are subject to block timing:
// 1 week at 3s/block
uint256 public constant EXIT_FUND_WITHDRAW_DELAY_IN_BLOCKS = (7 * 24 * 60 * 60) / 3;
// 1 day at 3s/block
uint256 public constant FIELD_UPGRADE_DELAY_IN_BLOCKS = (1 * 24 * 60 * 60) / 3;
// 1 week at 3s/block
uint256 public constant MAX_CHAIN_PROPAGATION_PERIOD_IN_BLOCKS = (7 * 24 * 60 * 60) / 3;
The division by 3 is derived from the average block time on Polygon, which is slightly higher than 2 seconds. The intention was to calculate delays so that they are at least the specified amount or longer (depending on block time) rather than always falling short.
However, this is not the case with the current implementation - as they are always falling short. Let’s consider the parameter FIELD_UPGRADE_DELAY_IN_BLOCKS that is used for delaying upgrades to critical configuration settings such as changing the insurance wallet or market settings:
(1 * 24 * 60 * 60) / 3, resulting in 28800 blocks that need to pass for finalizing the change.28000*2 seconds, resulting in 15.5 hours (instead of the actual 24 hours).This behavior of delays falling shorter than specified, can have quite severe implications for users, as they might e.g. want to close their positions before any change comes into effect.
Consider calculating above block delays by dividing by a value that is at maximum the average block time, preferable a bit smaller.
The targeted deployment chain does not have stable block times. As a result, we switched any logic that uses block numbers to timestamps.
For trades, withdrawals, and transfers, traders need to pay fees of up to 20% of the amount being moved. These fees are charged by IDEX to cover the incurred gas costs.
Despite this limit of 20%, traders have no control over the amount of fees they are willing to pay.
Hence, when traders perform one of the above operations, it is not transparent to them how much fees they gonna pay, and they can end up loosing 20% of the amount being moved.
Especially for withdrawals and transfers, where on-chain settlement happens almost immediately, the amount of gas required for the transaction can be predicted quite reliable. For such cases, traders want to have the transparency and control over the expected fees.
Consider one of the following remediations:
predictedFee amount to the signature message, so that traders can give their consent over paid fees when signing the message. The actual amount being paid on fees must then match the signed predictedFee amount, considering a certain tolerance. Fees are considerably more complex in Ikon than in many dapp trading protocols, such as swap AMMs. For example, fees vary between makers and takers, can be negative for promotional reasons, include variable gas, and can change over the course of trade executions against a single placed order. As such, providing a reasonable predictedFee is not straightforward and is a source of complexity we would like to avoid. Notably, the value of the fee cap is largely driven by gas concerns. In prior releases, the fee cap was configured at 20% to allow for gas collection during price spikes while maintaining low trade minimums. Of course, it is in the interest of neither the user nor the exchange to lose 20% of trade value from gas. Recent developments in blockchain deployment target performance indicate that settlement gas may be negligible for this release. In that case, we would lower that value, likely to something in the 1% to 5% range.
More context on the fix change:
The fee cap is updated to 5% and may be lowered based on the gas characteristics of the production deployment chain.
In MarketAdmin.publishIndexPrices_delegatecall, there is no sanity check for the passed index price. Propagating an incorrect index price can have severe consequences on all the other operations relying on the index price.
Note that the likelihood of providing an incorrect index price is considered low, as IDEX stated that they take several precautions in off-chain components to mitigate this case. However, the possibility of a compromised service wallet or a logic bug in off-chain components is not negligible.
Consider checking the provided index price is valid by e.g. checking against oracle price if the index price is too far off from previous value.
We deeply considered potential validations for index pricing and ultimately decided against their inclusion. Index prices are a critical system input for a perpetuals exchange, and accuracy, timing, and update frequency are all important requirements.
One potential validation scheme limits the rate of change of index prices over time. Crypto markets are volatile, of course, so picking limits that provide practical protection while allowing the necessary inputs during extraordinary events is a challenge. Further complicating the issue is Ikon’s lazy index price publishing system. If no activity happens in a market, and thus no price updates are published for an extended period of time, what change caps should apply?
Another potential scheme compares published index prices to another source, such as an on-chain oracle. Oracles are not perfect, however, and can themselves be wrong or delayed, likely under the very extraordinary market conditions when it’s most critical to apply up-to-date index pricing. Oracle price comparisons are also complicated by Ikon’s serialized dispatch model, both for out-of-date and up-to-date settlements. If dispatch is delayed, for example due to RPC issues, and settlement occurs an hour after trading, index prices from the time must be compared against matching historical oracle pricing. When dispatch is running normally, off-chain systems must accept propagation delays in on-chain pricing when deciding whether to accept a new index price update. Any conflict resulting from propagation time could interrupt operations.
While it is possible to pick an on-chain validation scheme, in our analysis such schemes introduce operational tradeoffs that can themselves increase risk for traders. Our design combines baseline validations, for example replay protection, while focusing engineering efforts in making index price collection as reliable, performant and secure as possible off chain.
More context on the fix change:
We modularized Ikon’s index price validation systems in an effort to diversify index prices sources. As a result, Ikon supports independent index price services with protocol-level validations, starting with Pyth Network. First party index prices are still supported for redundancy, subject to governance.
In Funding.publishFundingMultiplier_delegatecall, there is no sanity check for the passed funding rate. An incorrect provided funding rate can have severe consequences on the funding payments, which can lead to wallets ending up with high negative balance or others that receive much more in payments than intended.
Consider adding a minimum and maximum boundary to the funding rate.
Funding rates are clamped to 75% of the maintenance margin ratio of a market in our off-chain systems. Validating that funding rates are less than the maintenance margin ratio of a market also makes sense as a contract validation.
Validations.validateOverridableMarketFields lacks certain validation checks allowing to create and modify markets with undesirable behavior. For instance, markets could be created with maintenanceMarginFraction > 100%, making it possible to liquidate all positions.
Consider adding the following validation checks:
initialMarginFraction and maintenanceMarginFraction (e.g. ≤ 100%)incrementalInitialMarginFractionminimumPositionSize (currently it is set to uint64(type(int64).max - 1)))We considered adding more hard limits to market fields but chose the implemented governance protections instead. Putting aside legitimate scenarios where it may make sense to have 100% or even higher margin requirements, any change to these settings must be published on chain significantly before taking effect. Even if there were validation capping the maintenance margin fraction at say 50%, an attacker changing a market with a 5% mmr to a 50% mmr could have a significant adverse effect on many users. The upgrade governance mechanism, however, significantly reduces such risk.
For deleveraging “In Maintenance Acquisition”, the documentation states the following:
Validations confirm that the insurance fund cannot liquidate the wallet in maintenance via a standard Wallet In Maintenance liquidation.
However, when checking if the IF wallet can liquidate the wallet, the outstanding funding payments for the IF wallet are not taken into account. Thus, it becomes possible under specific circumstances, that a wallet is getting deleveraged via deleverageInMaintenanceAcquisition while standard liquidation via the IF wallet would have been possible.
The same issue is present with the deleverageExitAcquisition call.
While the likelihood of this is low (see response by IDEX), it breaks one of the protocols invariants and can lead to undesirable outcome.
Consider taking outstanding funding payments into account when validating that the insurance fund cannot acquire a position.
The following state-changing functions in Exchange.sol don’t emit events:
setCustodiansetDepositIndexsetDispatcher, removeDispatcheraddMarket, activateMarket, deactivateMarketunsetMarketOverridesForWalletskimIn addition, all the liquidation and deleverage functions don’t emit any events, which we assume is intentional to save gas costs for expected exchange activities.
Consider adding events for above functions.
The maximum value for the chain propagation period (MAX_CHAIN_PROPAGATION_PERIOD_IN_BLOCKS) is defined with 7 days. Especially for nonce invalidation - which should protect against “canceled-order submission attacks” - using a high value for the chainPropagationPeriodInBlocks parameter makes nonce invalidation rather ineffective.
Consider lowering the maximum allowed chainPropagationPeriodInBlocks to a much smaller value.
chainId in signature
The signature generation doesn’t include chainId parameter. This makes the protocol susceptible to replay attacks when the protocol is deployed to multiple chains or when a hard fork of the chain happens.
Consider including the chainId to the signature message.
Rather than using our own message format for signature creation, we decided to use EIP712 signature scheme (which protects against above attack vector).
withdrawExit from the IF wallet
Insurance fund is an important instrument in perpetual swaps to keep the exchange solvent whilst trading with leverage. In doing so, the insurance fund accumulates value over time under normal market conditions.
The insurance fund has the ability to close open positions at the market price via order book trades. However, under no circumstances should the insurance fund be able to withdrawExit and thus close its positions against the exitFund. This is guaranteed by the following check in Withdrawing.sol#L59:
require(wallet != insuranceFundWallet, "Cannot exit IF");
Consider the following scenario where an admin could run the following steps making it possible to withdrawExit from an IF wallet:
exitWallet() setting this new wallet to exit status.Following the above steps, an admin could now withdrawExit from insurance fund wallet.
Note that this attack vector is considered as low likelihood, however it needs to be taken into consideration that this can potentially damage the trustworthiness of the protocol.
Consider checking that the new insurance fund wallet hasn’t been exited before.
Preventing the insurance fund wallet from exiting is primarily an operational concern. An exited insurance fund wallet cannot participate in trade settlements to close positions, and the protocol assumes that the insurance fund wallet itself acquires the positions of an exited wallet during normal operations. While the outlined scenario is possible, it does not provide the attacker any profit angle due to exit pricing. That said, agreed that validating that the new insurance fund wallet is not exited in Governance.initiateInsuranceFundWalletUpgrade and Governance.finalizeInsuranceFundWalletUpgrade provides additional operational assurances.
It is advised to revert with custom errors over using require statements. Using them saves bytecode on deployment, a little gas on execution, and allows for more detailed error messages with the ability to pass in parameters.
Consider replacing require statements with custom errors to give more context about errors and save a bit of gas.
safeTransfer/safeTransferFrom
For deposits and withdrawals, transfer and transferFrom are used. It is advised to check return values for transfer and transferFrom or to use OpenZeppelin’s safeTransfer/safeTransferFrom.
Return value checks are not necessary due to the balance checks implemented.
withdrawNativeAsset
In ExchangeStargateAdapter.withdrawNativeAsset, there is no check if the provided destination wallet is address(0).
Consider adding a check destinationWallet ≠ address(0) to ensure no funds are lost by sending it to address(0).
When a new market is added in MarketAdmin.addMarket_delegatecall, the current timestamp is used for setting lastIndexPriceTimestampinMs. While this doesn’t impose any immediate security risks, it would be more accurate to use the updatedAt timestamp returned from Chainlink’s latestRoundData call (see here).
Market.lastIndexPriceTimestampinMs prevents the publishing of old index prices. The current timestamp provides a consistent up-to-date value, unlike oracle providers which may be out of date.
Moving funds via trades, withdrawals, and transfers, requires a valid signature from the trader. Currently, the protocol uses its own message format for signature creation.
Consider supporting signature creation according to the EIP712 standard for better security and usability.
sgReceive can be called by anyone
The sgReceive function in ExchangeStargateAdapter.sol is not restricted and can be called by anyone. If USDC tokens are left in the contract, anyone can call sgReceive to deposit the leftover tokens to their own wallet. However, this can be a desired behavior and doesn’t impose any security risks. Additionally, all the recommendations are met that are expected from a permissionless sgReceive function.
As stated in the documentation, the current version supports USDC as collateral only. Supporting any other collateral assets than USDC would require an upgrade of the Exchange contract.
USDC is supposed to be pegged to USD and usually variations in price between USDC and USD are small, but - as recent events have shown - it is never guaranteed that USDC doesn’t get depegged from USD and drops in value (see here).
Ikon supports any ERC-20 token as a collateral asset, but the collateral asset cannot be changed after deployment without an upgrade.
Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.
Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.
The scope of this report and review is limited to a review of only the code presented by the IDEX team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.