informal-report-injective-audit-202106

Security Audit Report

Injective Protocol: Protocol Design

Contents

Audit overview . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 4 The Project . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 4 Scope of this report . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 4 Conducted work . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 5 Findings . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 5 Audit Dashboard . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 7 Engagement Goals . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 9 Coverage . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 10 Recommendations . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 11 Short term . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 11 Long term . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 11 Findings . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 12 CLI interface fails with a stack trace when supplying incorrect arguments #287 . 14 Hard-coded fee recipient in the client code #289 . . . . . . . . . . . . . . . . . 15 Missing validation tests in MsgInstantSpotMarketLaunch, results in division by zero #291 . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 16 NPE when launching an instant spot market #295 . . . . . . . . . . . . . . . . 18 Outdated parameters in scripts/propose_spot_market.sh #296 . . . . . . . . . 19 Incorrect parsing of arguments for injectived tx exchange create-spot-market-order #299 . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 20 Changing the status of a spot market to Demolished introduces a market copy #302 21 When a spot market is demolished the outstanding sell orders (and their coins) are frozen #304 . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 23 CLI for launching derivative markets #322 . . . . . . . . . . . . . . . . . . . . 25 A sequence of transactions leads to a complete halt of consensus #323 . . . . . 26 Price feed does not validate prices, may crash consensus #331 . . . . . . . . . . 28 Recommendation for recovery in EndBlocker #301 . . . . . . . . . . . . . . . . 33

Audit overview

The Project In May 2021, Injective engaged Informal Systems to conduct a security audit over the documentation and the current state of the implementation of Injective Protocol: a Cosmos-backed decentralized derivatives trading platform. The agreed-upon workplan consisted of two steps:

Milestone 1: Reviewing spot markets The focus of this milestone was to review the code that implements exchange in the spot markets. As spot markets are relatively simple, we agreed that it was a good starting point. The input to this milestone was: documentation on Notion, code walkthrough, the codebase in the private github repository called injective-core. Deliverables include open issues that describe functional and security bugs as well as TLA+ specifications, which can be used for model-based testing. In this milestone, we mainly focused on the audit of the exchange module.

Milestone 2: Reviewing derivative markets The implementation of the derivative markets is more sophisticated in comparison to the spot markets. The input to this milestone was: documentation on Notion, the codebase in the private github repository called injective-core. Deliverables include open issues that describe functional and security bugs as well as TLA+ specifications, which can be used for model-based testing. In this milestone, we mainly focused on the audit of the modules: exchange, oracle, and insurance.

Scope of this report This report covers the audit in the framework of Milestones 1-2 that was conducted May 11 through June 14, 2021 by Informal Systems under the lead of Igor Konnov, with the support of Zarko Milosevic. The team spent 3 person-weeks on the audit. As the codebase spans over 38 KLOC of Golang code, we could not perform an exhaustive audit of the whole codebase. Rather we have identified potential problems in the code and tried to trigger critical errors in the system.

Conducted work Starting May 11, the Informal Systems team conducted an audit of the existing documentation and code in the project directory in the Cosmos repository of hash 4dac628e. The Injective Labs team was resolving issues that were blocking our further progress. Hence, we continued with more recent versions of the development branch. The most important issues we documented in the findings which are part of this report, and as issues on the Injective Labs GitHub repository. A detailed list can be found in the Findings. As we quickly found that the general code quality was high and the Injective Labs team tested their code on regular basis, we changed our auditing approach to model-based testing, which was backed by a symbolic model checker. To this end, we have designed high-level specifications of spot markets and derivative markets in TLA+, by following the English specifications that were provided by Injective Labs. Importantly, our TLA+ specifications do not focus on complete functional correctness. Rather, we used them to drive the system into a potentially problematic state that we could manually inspect, in order to trigger bugs in the system.

Findings We have found that Injective Protocol is written with attention to details. Large parts of the codebase contain all necessary validation tests and do not let an attacker to easily exploit overflows, replay previously recorded transactions or perform timing attacks. As a result, our straightforward attempts to attack the system did not succeed. As we switched to semi-automated model-based testing, we found issues with the command-line interface of the Injective Protocol, all resolved: • IF-INJECTIVE-01, • IF-INJECTIVE-04, • IF-INJECTIVE-05, • IF-INJECTIVE-06, • IF-INJECTIVE-09. None of these issues is severe, as they only affected the client interface. The main reason for the team paying less attention to CLI is that they are testing their system by running end-to-end integration tests (that do not use CLI) as well as manual testing via the web interface. The Injective Labs team was surprisingly responsive in fixing the discovered issues. Usually, they fixed

issues in less than 1 hour after receiving a report on GitHub and the Discord channel. Hence, although CLI issues slightly impeded our progress, they did not block us. By code inspection, we have found that Injective Protocol implements reach functionality in abci.go:BeginBlocker and abci.go:EndBlocker. While errors in Cosmos transactions are automatically recovered by the Cosmos framework, by rolling back an offending transaction, errors in BeginBlock and EndBlock are not automatically recovered. Every such an error results in halting the consensus engine, which effectively means that all validators would have to patch the code and to coordinate in restarting the blockchain. We have documented this potential issue in IF-INJECTIVE-12. The team has confirmed that this indeed a potential severe issue that requires careful redesign of the code. Later, we indeed found attack vectors IF-INJECTIVE-10 and IF-INJECTIVE-11 that exploited this issue. We believe that these are only two instances of the general issue IF-INJECTIVE-12. Hence, the issues IF-INJECTIVE-10, IF-INJECTIVE-11, and IF-INJECTIVE-12 are the most severe. We recommend designing good defense mechanisms against them. Both issues 10 and 11 highlight interesting sources of errors, to which the team should pay further attention: • IF-INJECTIVE-10 was triggered after market expiration, which could potentially last for weeks or months in production. Interestingly, the user had only to launch a market and wait, without performing any trading activity. Resolved. • IF-INJECTIVE-11 was triggered by corrupt input from a price feed. As price feeds are outside of the designer’s control, we recommended the team to carefully validate and filter price feeds. Resolved. Two further issues were less severe, but they could probably result in fraud or loss of tokens: • In issue IF-INJECTIVE-07, changing the status of a spot market resulted in launching another market instance. Resolved. • In issue IF-INJECTIVE-08, demolishing a spot market resulted in outstanding orders (and their tokens) being frozen. Resolved. Finally, we found two non-critical issues: • Transactions invoked by CLI contained a hard-coded recipient address: IF-INJECTIVE-02. Resolved. • Transaction panic IF-INJECTIVE-03. Resolved. We emphasize that the five severe issues would not be found by the standard lightweight static analysis or fuzzing. They required knowledge of the source code and executing carefully crafted sequences of transactions. We do not consider them as being easily exploitable.

Audit Dashboard Target Summary • Name: Injective Protocol • Version: 4dac628eb1d08f4d66685e9f228f6ff53e9197c9 through baa69e1c366e9dc8727c7385fa120c08162b0 • Type: Implementation and preliminary documentation • Platform: Golang Engagement Summary • Dates: May 11 through June 14, 2021 (kick-off meeting May 7) • Method: Whitebox, model-based testing, symbolic model checking • Employees Engaged: 2 • Time Spent: 21 person days Fundings Summary by Severity and Difficulty

Severity Difficulty # Finding High Low 1 IF-INJECTIVE-10 High High 2 IF-INJECTIVE-11, IF-INJECTIVE-12 Medium Medium 2 IF-INJECTIVE-07, IF-INJECTIVE-08 Low Low 7 IF-INJECTIVE-02, IF-INJECTIVE-03, IF-INJECTIVE-01, IF-INJECTIVE-04, IF-INJECTIVE-05, IF-INJECTIVE-06, IF-INJECTIVE-09 Total 12

Category Breakdown

Finding Type # Distributed System Reliability and Fault Tolerance 3 Protocol, Economics & Implementation 3 Implementation & Testing 6

Finding Type # Total 12

Severity Categories

Severity Description Informational The issue does not pose an immediate risk (it is subjective in nature); they are typically suggestions around best practices or readability Low The issue is objective in nature, but the security risk is relatively small or does not represent security vulnerability Medium The issue is a security vulnerability that may not be directly exploitable or may require certain complex conditions in order to be exploited High The issue is exploitable security vulnerability

Difficulty Categories

Difficulty Description Low Can be attacked by a user without special permission Medium Can be exploited without special permission with in-depth knowledge and control of the security architecture High Needs a collection of privileged users with in-depth knowledge and control of the security architecture

Engagement Goals This audit was scoped by the Informal Systems team in order to assess the correctness and security of the Injective Protocol. It was planned along two Milestones. In the first milestone, the focus was to get familiarized with the codebase and look for potential attack vectors in the spot markets. In the second milestone, the focus was on derivative markets. As the scope of the project is too large for a short-term audit, we agreed that it was not feasible to acquire comprehensive understanding of the protocols and system functionality. Instead, we focused on potential attack scenarios by inspecting the code and running model-based tests.

Coverage Informal Systems manually reviewed the documentation and code of the software in the In- jective Chain directory starting at commit hash 4dac628eb1d08f4d66685e9f228f6ff53e9197c9. As the code was updated during the review, we continued with further commits through baa69e1c366e9dc8727c7385fa120c08162b08e0. We focused on the backend Cosmos code in the modules: exchange, insurance, and oracle. As the codebase spans over 38 KLOC of Golang code, we could not perform an exhaustive audit of the whole codebase.

Recommendations This section aggregates all the recommendations made during the audit. Short-term recommen- dations address the immediate causes of issues. Long-term recommendations pertain to the development process and long-term design goals.

Short term • Test for non-standard scenarios. Issues IF-INJECTIVE-07 and IF-INJECTIVE-08 were probably outside of the standard scenarios of a testing engineer, as they are rarely used features. • Test for time-related issues. As exemplified by IF-INJECTIVE-10, functional tests are not sufficient. Injective Protocol is using timeouts and hence it should be tested with timeouts in mind. • Avoid hard-coded addresses. As exemplified by IF-INJECTIVE-02. • Add tests for CLI. As demonstrated by issues IF-INJECTIVE-01, IF-INJECTIVE-04, IF-INJECTIVE-05, IF-INJECTIVE-06, IF-INJECTIVE-09.

Long term • ABCI methods. As stressed in IF-INJECTIVE-12 and exemplified by IF-INJECTIVE-10, the system should be re-designed in a way that does not halt the consensus engine, if a panic occurs in the code that is triggered by the methods abci.go:BeginBlock and abci.go:EndBlock. • Price oracles. As exemplified by IF-INJECTIVE-11, the team should pay attention to the interaction with the price oracles, as they are outside of the designer’s control. Thus, price oracles may be used by an attacker or they can accidentally feed corrupt data into the system. We recommend receiving price values from 3f + 1 feeds and filtering out the f smallest and the f largest values, while averaging the rest.

Findings

Title Type Severity Issue

IF-INJECTIVE-10 A sequence of Distributed High 323 transactions leads System Reliability to a complete halt and Fault of consensus Tolerance IF-INJECTIVE-11 Price feed does Distributed High 331 not validate prices, System Reliability may crash and Fault consensus Tolerance IF-INJECTIVE-12 Recommendation Distributed High 301 for recovery in System Reliability EndBlocker and Fault Tolerance IF-INJECTIVE-07 Changing the Protocol, Medium 302 status of a spot Economics & market to Implementation Demolished introduces a market copy IF-INJECTIVE-08 When a spot Protocol, Medium 304 market is Economics & demolished the Implementation outstanding sell orders (and their coins) are frozen IF-INJECTIVE-01 CLI interface fails Implementation & Low 287 with a stack trace Testing when supplying incorrect arguments IF-INJECTIVE-02 Hard-coded fee Protocol, Low 289 recipient in the Economics & client code Implementation

Title Type Severity Issue

IF-INJECTIVE-03 Missing validation Implementation & Low 291 tests in Testing MsgInstantSpot- MarketLaunch, results in division by zero IF-INJECTIVE-04 NPE when Implementation & Low 295 launching an Testing instant spot market IF-INJECTIVE-05 Outdated Implementation & Low 296 parameters in Testing scripts/propose_spot_market.sh IF-INJECTIVE-06 Incorrect parsing Implementation & Low 299 of arguments for Testing injectived tx exchange create- spot-market-order IF-INJECTIVE-09 CLI for launching Implementation & Low 322 derivative markets Testing

IF-INJECTIVE-01

CLI interface fails with a stack trace when supplying incor- rect arguments #287 Status: Resolved Severity: Low Type: Implementation & Testing Difficulty: Low Surfaced from Informal Systems audit at hash 4dac628eb1d08f4d66685e9f228f6ff53e9197c9. Observed behavior Here is an example for “query exchange deposits”: ~/go/bin/injectived query exchange deposits panic: runtime error: index out of range [1] with length 0 ... Expected behavior An error message printed on stderr, without a stack trace. For instance, here is how the bank module reacts on the wrong number of arguments: ~/go/bin/injectived query bank balances Error: accepts 1 arg(s), received 0 Usage: injectived query bank balances [address] [flags] ... Version Running injectived that was compiled from 4dac628eb1d08f4d66685e9f228f6ff53e9197c9.

IF-INJECTIVE-02

Hard-coded fee recipient in the client code #289 Status: Resolved Severity: Low Type: Protocol, Economics & Implementation Difficulty: Low Surfaced from Informal Systems audit of hash 4dac628eb1d08f4d66685e9f228f6ff53e9197c9 This is issue has been created for documentation purposes. It has been fixed by the team in 043a2402e59a985400c676dc6d4b6fa1ca85567b after communication on discord. The client code contained a hard-coded address of the fee recipient: https://github.com/InjectiveLabs/injective- core/blob/4dac628eb1d08f4d66685e9f228f6ff53e9197c9/injective-chain/modules/exchange/client/cli/tx.go#L174 L188 The fix https://github.com/InjectiveLabs/injective-core/commit/043a2402e59a985400c676dc6d4b6fa1ca85567b sets the sender as the fee recipient.

IF-INJECTIVE-03

Missing validation tests in MsgInstantSpotMarketLaunch, results in division by zero #291 Status: Resolved Severity: Low Type: Implementation & Testing Difficulty: Low Surfaced from Informal Systems audit of hash 043a2402e59a985400c676dc6d4b6fa1ca85567b It is possible to launch a market with incorrect parameters that results in a transaction panic later. Consider the following sequence of commands in the standard setup, as done with ./setup.sh: injectived tx exchange instant-spot-market-launch INJ/INJ inj inj
--from=genesis --chain-id=888 --keyring-backend=file injectived tx exchange deposit 10000000inj --chain-id 888
--from inj1cml96vmptgw99syqrrz8az79xer2pcgp0a885r injectived tx exchange create-spot-limit-order buy INJ/INJ 10 10
--from=user1 --chain-id=888 --keyring-backend=file This leads to a transaction panic: {"height":"2318",[...] to execute message; message index: 0: division by zero: panic","logs":[],"info":"","gas_wanted":"200000","gas_used":"64317", "tx":null,"timestamp":""} The reason is that the market is launched with min_price_tick_size = 0: injectived query exchange spot-markets markets:

base_denom: inj maker_fee_rate: "0.001000000000000000" market_id: 0x3b78a9b8efc920e7021cc30cb3c821df189585cc3eaa35d73ec8853a1780961d min_price_tick_size: "0.000000000000000000" min_quantity_tick_size: "0.000000000000000000" quote_denom: inj relayer_fee_share_rate: "1.000000000000000000" status: Active

taker_fee_rate: "0.002000000000000000" ticker: INJ/INJ

IF-INJECTIVE-04

NPE when launching an instant spot market #295 Status: Resolved Severity: Low Type: Implementation & Testing Difficulty: Low Surfaced from Informal Systems audit of hash 1e4d2914b3ae616b98b05fb70eb487550fc99ed7 This is a follow up of #291. The recent fix in ba9f2eb7a76dfd81a9d1f74970597085e2253357 introduced NPE in the client. run the following command in the standard setup, as done with ./setup.sh: injectived tx exchange instant-spot-market-launch INJ/INJ inj inj
--from=genesis --chain-id=888 --keyring-backend=file Enter keyring passphrase: panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x5fb461]

... As far as I can tell, the code in injective-chain/modules/exchange/client/cli/tx.go fails to add the message fields MinPriceTickSize and MinQuantityTickSize, which results in an NPE later.

Excerpt (19991 of 56566 characters). Read the whole page on informalsystems/audits ↗

← Back to Documentation