Skip to main content
Dedaub

Arrow.markets

Smart Contract Security Assessment

Jan 17, 2022

Arrow

SUMMARY


ABSTRACT

Dedaub was commissioned to perform a security assessment of several smart contract modules of the (currently in-development) Arrow.markets protocol.

The audit methodology involved careful review of (currently closed-source) sources, static analysis of compiled contracts and inspection of off-chain systems expected to interact with the Arrow.


SETTING AND CAVEATS

The audit was conducted on an in-development version of Arrow. This has allowed Dedaub to make suggestions at an early stage on what to look out for whilst developing the system. On the other hand, at this stage of development the system is not yet fully functional and lacks a complete test suite. Although a security audit may reveal a number of functional issues, it is more intended on finding security issues in an adversarial environment. In order to find functional issues, further testing of the application under many different settings needs to be conducted. There is also an additional risk that new bugs may be introduced before the final project is deployed. Further audits for the rest of the project after testing and staging, but prior to final deployment will considerably lower these risks.

The main threat with the architecture of Arrow is a centralized component that drives the key functions of the on-chain smart contract. The centralized component, referred to as an Oracle in the documentation, does more than calculate option prices - all important commands to the smart contract such as buying or selling options go through it and in the current design its own internal database maintains the true state of the market it controls. Although an important aspect, this is not the only centralization component in this protocol. For example, the owner can mint or burn as many options as they require.

The need for a centralized component is motivated by the complexity of the options’ pricing calculation and the hedging strategy that the protocol adopts, which, if works as intended, would result in better capital efficiency than existing decentralized options protocols we are currently aware of. It is clear that the Arrow development team is well-versed in the economic design of derivatives protocols, and have made important decisions early on to maximize the efficiency. A “Delta-hedging” strategy, which is driven by a centralized calculation of how much is the underlying protocol exposed to long and short price fluctuations (via calls and puts respectively) minimizes the risk by trading the stable asset for the volatile asset. In some cases, it will also borrow the volatile asset through BenQi and sell it to achieve a short position.

The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than regular use of the protocol. Functional correctness (i.e., issues in "regular use") is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e., full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling, quantities returned from external protocols) is generally most effectively done through thorough testing.


VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues that affect the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • User or system funds can be lost when third-party systems misbehave.
  • DoS, under specific conditions.
  • Part of the functionality becomes unusable due to a programming error.
LOW
Examples:
  • Breaking important system invariants but without apparent consequences.
  • Buggy functionality for trusted users where a workaround exists.
  • Security issues which may manifest when the system evolves.

Issue resolution includes “dismissed”, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

[No critical severity issues]


HIGH SEVERITY

H1

Borrowed Assets are Never Repaid

H1HIGH

Borrowed Assets are Never Repaid
open

The delta hedging strategy sometimes needs to borrow volatile assets from BenQi, putting stablecoins as collateral, and sell these volatile assets in order to enter into a short position. A short position of this sort would counter a large number of PUT option positions that the protocol would be exposed to. However, there is no functionality in place to repay the loan, which can be regarded as essential protocol functionality.

H2

Missing Modifiers

H2HIGH

Missing Modifiers
open

There are many important places where modifiers are not yet used. Listing some of these for reference:


ArrowOptionChainProxy::deliverOption

ArrowOptionChainProxy::incrementFees

ArrowOptionChainProxy::initSettlement

ArrowOptionChainProxy::settle

ArrowRouter::capitalizeInsurancePool

ArrowRouter::capitalizePrimaryPool

ArrowEvents::emitTransfer

ArrowInsurancePool::setArrowToken
ArrowInsurancePool::subsidizeArrowChain

ArrowInsurancePool::capitalizePrimaryPool
ArrowInsurancePool::redeem

H3

Central protocol functionality is not implemented

H3HIGH

Central protocol functionality is not implemented
open

A few contracts are lacking a complete implementation, meaning that further development is needed, which could render the current security assessment partially insufficient. Examples of contracts that are not complete are the ArrowHedgerDelegate contract, which does not implement the logic to acquire collateral in cases where the current balance is not sufficient to borrow enough of the underlying asset, and the ArrowInsurancePool contract which is missing functionality such as LP shares/rewards and primary pool capitalization.



MEDIUM SEVERITY

M1

Pangolin swaps might be prone to front-running attacks

M1MEDIUM

Pangolin swaps might be prone to front-running attacks
open

In calls to swapExactTokensForTokens of the Pangolin protocol, the minAmountOut parameter, which describes the minimum amount of received tokens that is expected for the swap to not revert, is set to 0. On the Ethereum blockchain such swaps can be easily front-runned as they offer no front-running protection. The Avalanche blockchain uses a leaderless consensus algorithm and has significantly shorter block confirmation time, still it appears that MEV attacks are real and countermeasures should be put in place to protect protocol users.

M2

Incorrect update process of protocol parameters can lead to invalid protocol state

M2MEDIUM

Incorrect update process of protocol parameters can lead to invalid protocol state
open

Several key storage variables are duplicated among protocol contracts. This means that when upgrading such a variable we need to interact with multiple smart contracts. However, only the ArrowRouter allows the owner to choose the new value, while all other contracts need to consult the ArrowRouter contract through their update functions to get informed of this new value. It is critical that in cases where multiple updates of the “same” storage variable across different contracts need to happen, these are done in “one” transaction by using a smart contract. Otherwise, if each update happens in a separate transaction the protocol will exist in an invalid state.

Invalid state example: The ArrowRegistry address is updated in the ArrowRouter contract. The registry address of the ArrowChainFactory contract also needs to get updated but the updating transaction is preceded by a user deliverOption transaction. The requested ArrowOptionContract does not exist in the new ArrowRegistry, thus the ArrowChainFactory is called, which calls and updates the stale ArrowRegistry.

M3

Possible Race Conditions for Off-Chain Component

M3MEDIUM

Possible Race Conditions for Off-Chain Component
open

The off-chain oracle intends to either (a) listen to events emitted on-chain, and adjust its own state based on this; or (b) maintain its own state based on its own past transactions. Unfortunately, due to soft forks or race conditions on the network, the state of the off-chain component may not reflect the actual state on-chain which means that if the oracle adjusts its price based off of recent transactions, an attacker could potentially exploit the system if, for instance, a large transaction which manipulates the price is undoed, or if a large transaction is not accounted for in the network partition visible to the on-chain component.



LOW SEVERITY

L1

Missing address check in ArrowRouter::settleOption

L1LOW

Missing address check in ArrowRouter::settleOption
open

In method ArrowRouter::settleOption the registry contract is consulted to retrieve the correct ArrowOptionChain address. However, there is no check after the call that verifies the returned address is not 0 in case wrong parameters are provided. In such a case the transaction will revert without providing adequate information to the user.

L2

ArrowHedgerDelegate is prone to storage collision

L2LOW

ArrowHedgerDelegate is prone to storage collision
open

The ArrowHedgerDelegate contract does not share the same storage layout with the ArrowOptionChainDelegate contract, which can delegatecall into it. Fortunately, ArrowHedgerDelegate defines a single storage variable, hasEnteredMarket, which is never used. Nevertheless, storage collisions might get introduced if ArrowHedgerDelegate is extended in a way that does not take into account storage layout and the delegate call from ArrowOptionChainDelegate.

L3

AERC20 inconsistent owners tracking

L3LOW

AERC20 inconsistent owners tracking
open

The AERC20 token overrides method _beforeTokenTransfer as shown in the code snippet below to track new owners of the token when a transfer happens.

function _beforeTokenTransfer(address, address _to, uint256) internal virtual override {
// super._beforeTokenTransfer(_from, _to, _amount);
if (!isOwner[_to] && _to != address(0)) {
owners.push(_to);
isOwner[_to] = true;
}
// Dedaub: from address ownership is disregarded
}

Nevertheless, the fact that the ownership status of the from address is not updated in case it does not hold a balance after the transfer means that the query isOwner[from] will return true and the addresses array returned by getOwners will contain from even if from is not an owner of the token any more.

In addition:

  • The function in the superclass is not called, which may be an issue if the underlying OpenZeppelin library evolves.
  • Tracking all owners is costly in terms of gas and there is no client code that reads from the isOwner[...] map.

L4

ArrowUtils::retrieveAddressFromSignature error handling is incomplete

L4LOW

ArrowUtils::retrieveAddressFromSignature error handling is incomplete
open

The address computation checks the length of the signature and if it is not 65 characters long it sets the deliveryAddress variable to 0 without returning it. We believe that the initial intent was to return the 0 address when the aforementioned check fails, as in the current version of the code the computation continues to compute r, s and v variables from a possibly incomplete signature and then sets again the deliveryAddress disregarding the previously set 0 value.

Also, according to Appendix F in the Ethereum Yellow paper the valid range for s is 0 < s < secp256k1n ÷ 2 + 1. This can be enforced with the following check, which has been borrowed from the OpenZeppelin ECDSA library code.

if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0)

L5

ArrowRegistry ticker inconsistency

L5LOW

ArrowRegistry ticker inconsistency
open

Asset addresses for tickers in the ArrowRegistry contract can only be set from the RouterOwner by calling the ArrowRegistry::setAssetAddresses. This method needs to be called for an asset before an ArrowOptionChainProxy is created for it, as otherwise the call ArrowChainFactory::createNewChain will fail. At the same time, the ticker for that asset is added to the registry when createNewChain calls ArrowRegistry::appendTicker. This means that in certain scenarios the call ArrowRegistry::getTickers might return an array that does not include ticker, while ArrowRegistry::getUnderlyingAssetAddress(ticker) will return the asset address for the ticker that appears to not exist in the registry’s tickers. This inconsistency might not be affecting the protocol directly, but it can be easily fixed by having the method setAssetAddresses call appendTicker.



OTHER/ ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.

A1

Deletion of local variables

A1ADVISORY

Deletion of local variables
open

In several places in the protocol’s contracts local variables are deleted. Deletion of local variables is not equal to deletion of storage variables, which results in a partial refund of the gas used for the initial store to incentivize programmers to be more mindful of persistent contract memory. On the contrary, deletion of local variables incurs a slight gas cost increase and thus should be avoided if there is no other reason to back it up.

A2

Duplicate external calls to registry

A2ADVISORY

Duplicate external calls to registry
open

Method _deliverOptionBuy of ArrowOptionChainDelegate calls registry’s method setContractDataToOptionContract after the internal call to createNewOption, which also calls the setContractDataToOptionContract method of ArrowRegistry.

Also, in method ArrowOptionChainDelegate::createNewOption the calls to registry’s method setContractDataToOptionContract and appendOptionAddress are duplicated, as they are also performed by the call to method createNewOption of ArrowOptionFactoryDelegate.

A3

Dead code

A3ADVISORY

Dead code
open

Method ArrowOptionChainDelegate::getTotalLiabilities is private and at the same time is not called by any other method of the contract.

A4

Unused storage fields

A4ADVISORY

Unused storage fields
open

Storage field hasEnteredMarket of IArrowOptionChain and ArrowHedgerDelegate is not used and can be removed.

A5

ERC20 token implementation deviates from the standard OpenZeppelin implementation

A5ADVISORY

ERC20 token implementation deviates from the standard OpenZeppelin implementation
open

It appears that over the course of the development of the protocol the ERC20 code was updated manually to follow the standard implementation from OpenZeppelin, which had been initially forked. However, this has resulted in trivial differences between the two implementations, which do not pose any risk to the protocol at the moment but at the same do not bring any merit. Our advice is to upgrade to the latest implementation published by OpenZeppelin.

A6

Superfluous proxy-delegate logic

A6ADVISORY

Superfluous proxy-delegate logic
open

The ArrowOptionChainProxy contract defines several methods that delegatecall to the respective method of the ArrowOptionChainDelegate contract. These methods differ only in name, parameters and the method they delegatecall, while the rest of the logic is the same. These methods can be replaced with a fallback function that uses calldata to delegatecall to ArrowOptionChainDelegate without being specific. That way the code is greatly simplified and smart contract upgradeability can be easily supported in future versions.

A7

ArrowOptionContract::initialize logic can be moved to the constructor

A7ADVISORY

ArrowOptionContract::initialize logic can be moved to the constructor
open

ArrowOptionContracts are created using CREATE2 as it gives the ability to predict the address where the contract will be deployed. For ArrowOptionContracts the protocol creators do not want to parametrize the deployment address by all the constructor parameters (creation bytecode is taken into account by CREATE2), thus they choose to use a placeholder constructor and an initializer function that has all the ArrowOptionContract initialization logic. This is a valid pattern but the initializer function might be introducing some unnecessary complexity. A lighter pattern, which is also used for the construction of ArrowOptionChainProxy contracts, employs the storage of the deployer contract to store the construction parameters, so that the newly deployed contract can read them. When the CREATE2 call returns the deployer is free to delete the associated storage and get back a gas refund for doing so. Applying this pattern to ArrowOptionFactoryDelegate would be best done by also removing the delegate part from the equation, thus avoiding storage layout/collision issues and the shortcomings of the low-level delegate call.

A8

Missing string validation

A8ADVISORY

Missing string validation
open

We recommend that ArrowOptionChainDelegate::createNewOption performs validation using Strings.sol on the decimal strike price string argument, or perhaps omits such an argument as it is only required for presentation purposes.

A9

Naming conventions for contracts

A9ADVISORY

Naming conventions for contracts
open

Base contracts have names starting with “I”, which would give the message that they are interfaces. However they contain important concrete logic. We recommend using different prefixes for such contracts and also moving logic, which does not lead to code duplication, to the implementation contracts.

A10

Floating version pragma in contracts

A10ADVISORY

Floating version pragma in contracts
info

The floating version pragma solidity >=0.8.0 <0.9.0 is used allowing the contracts to be compiled with any version from 0.8.0 until 0.9.0 of the Solidity compiler. Floating pragmas should be avoided and the pragma should be fixed to the version that will be used for the contracts’ deployment, even though versions might not differ drastically.

A11

Compiler bugs

A11ADVISORY

Compiler bugs
info

The contracts can be compiled with the Solidity compilers v0.8.0-v0.8.11. At the time of writing some of these versions exhibit a subset of 4 known issues. We have reviewed the issues and do not believe them to affect the contracts. More specifically the known compiler bugs associated with the aforementioned Solidity compiler versions are:

  • Memory layout corruption can happen when using abi.decode for the deserialization of two-dimensional arrays.
  • For immutable variables of a signed integer type shorter than 256 bits, sign extension of its value is not always properly performed. According to the Solidity team the value can only be accessed in its unclean state when using inline assembly.
  • The compiler does not correctly compute the storage layout of user defined value types for types that are shorter than 32 bytes, always using a full storage slot for these types, even if the underlying type is shorter.
  • The bytecode optimizer will incorrectly re-use previously evaluated Keccak-256 hashes under certain scenarios.


DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.


ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.

Dedaub