Arrow.markets

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.

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

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.

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.

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.

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.

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.

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.

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.

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)

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.

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.

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.

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

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

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.

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.

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.

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.

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.

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.

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.