Disclaimer

# 1. Introduction
iosiro was commissioned by [1inch Network](https://1inch.io)  to conduct an audit on their limit order smart contracts. The audit was performed by two auditors between 24 March and 13 April 2021, consuming a total of 16 resource days. A review was performed on 28 May 2021 reviewing changes made to the contracts to address the findings of the audit.

This report is organized into the following sections.

* **[Section 2 - Executive Summary:](#section-2)** A high-level description of the findings of the audit.
* **[Section 3 - Audit Details:](#section-3)** A description of the scope and methodology of the audit.
* **[Section 4 - Design Specification:](#section-4)** An outline of the intended functionality of the smart contracts.
* **[Section 5 - Detailed Findings:](#section-5)** Detailed descriptions of the findings of the audit.

The information in this report should be used to better understand the risk exposure of the smart contracts, and as a guide to improving the security posture of the smart contracts by remediating issues identified. The results of this audit are only a reflection of the source code reviewed at the time of the audit and of the source code that was determined to be in-scope.

The purpose of this audit was to achieve the following:

* Identify potential security flaws.
* Ensure that the smart contracts functioned according to the documentation provided.

Assessing the economics, game theory, or underlying business model of the platform were strictly beyond the scope of this audit.

Due to the unregulated nature and ease of transfer of cryptocurrencies, operations that store or interact with these assets are considered very high risk with regards to cyber attacks. As such, the highest level of security should be observed when interacting with these assets. This requires a forward-thinking approach, which takes into account the new and experimental nature of blockchain technologies. Strategies that should be used to encourage secure code development include:

* Security should be integrated into the development lifecycle and the level of perceived security should not be limited to a single code audit.
* Defensive programming should be employed to account for unforeseen circumstances.
* Current best practices should be followed where possible.

<a name="section-2"></a>
# 2. Executive Summary

This report presents the findings of an audit performed by iosiro on the [1inch Network](https://1inch.io) limit order smart contracts. The contracts allow users to create orders off-chain, which are then filled by one or more order takers. Additional functionality is available to constrain orders to certain conditions, such as an expiry time.

One high risk issue and one medium risk issue were identified. The high risk issue was fixed by the conclusion of the audit and related to a malicious maker potentially being able to front-run an order taker to manipulate the taking or making amounts. The medium risk issue related to a rounding issue leading to an excessive amount of assets taken from the order taker. Overall, the code was found to be of a high standard and accord to the specification provided.

<a name="section-3"></a>
# 3. Audit Details
## 3.1 Scope
The source code considered in-scope for the assessment is described below. Code from all other files is considered to be out-of-scope. Out-of-scope code that interacts with in-scope code is assumed to function as intended and introduce no functional or security vulnerabilities for the purposes of this audit.

### 3.1.1 1inch Smart Contracts
**Project Name:** limit-order-protocol<br/>
**Commit:** [fc528b3](https://github.com/1inch/limit-order-protocol/tree/fc528b390bad66927b9316470e4c86d06df58563), [1f97b54](https://github.com/1inch/limit-order-protocol/commit/1f97b5415621444b2b428f0a3e8a2cd10e5a65a2)<br/>
**File(s):** LimitOrderProtocol.sol, libraries/ArgumentsDecoder.sol, libraries/UncheckedAddress.sol, helpers/ERC20Proxy.sol, helpers/ERC721Proxy.sol, helpers/ImmutableOwner.sol, helpers/NonceManager.sol, helpers/PredicateHelper.sol

## 3.2  Methodology

A variety of techniques were used while conducting the audit. These techniques are briefly described below.

### 3.2.1 Code Review

The source code was manually inspected to identify potential security flaws. Code review is a useful approach for detecting security flaws, discrepancies between the specification and implementation, design improvements, and high risk areas of the system.

### 3.2.2 Dynamic Analysis

The contracts were compiled, deployed, and tested in a Ganache test environment. Manual analysis was used to confirm that the code operated at a functional level, and to verify the exploitability of any potential security issues identified.

### 3.2.3 Automated Analysis

Tools were used to automatically detect the presence of several types of security vulnerabilities, including reentrancy, timestamp dependency bugs, and transaction-ordering dependency bugs. The static analysis results were manually analysed to remove false positive results. True positive results would be indicated in this report. Static analysis tools commonly used include Slither, MythX, as well as Securify. Furthermore, the Remix IDE, compilation output, and linters are also used to identify potential areas of concern.

## 3.3  Risk Ratings

Each issue identified during the audit has been assigned a risk rating. The rating is determined based on the criteria outlined below.

* **High Risk** - The issue could result in a loss of funds for the contract owner or system users.
* **Medium Risk** - The issue resulted in the code specification being implemented incorrectly.
* **Low Risk** - A best practice or design issue that could affect the security of the contract.
* **Informational** - A lapse in best practice or a suboptimal design pattern that has a minimal risk of affecting the security of the contract.
* **Closed** - The issue was identified during the audit and has since been addressed to a satisfactory level to remove the risk that it posed.

<a name="section-4"></a>
# 4. Design Specification
The following section outlines the intended functionality of the system at a high level.

## 4.1 Limit order protocol

The `LimitOrderProtocol` contract was the central contract for handling and filling orders. Makers looking to sell their tokens can create orders off-chain, which then appear on-chain when filled or cancelled. Orders can be created with ERC-20, ERC-721 or ERC-1155 tokens as the maker and taker assets.

The limit order protocol includes functionality for conditional trades, which require certain prerequisites to be met before the order can be filled. These conditions are set my the order maker, and can be several conditions chained together such as order expiry and wallet balance conditions. Orders also have a post-order hook, which allows the order maker to execute certain code once the order has been at least partially filled.

Orders can also be private by specifying the taker address when creating the order off-chain, which prevents any other user from filling the order.

<a name="section-5"></a>
# 5. Detailed Findings
The following section includes in depth descriptions of the findings of the audit.

## 5.1 High Risk

No high risk issues were present at the conclusion of the audit.

## 5.2 Medium Risk

### 5.2.1 Rounding error leads to additional assets taken from order taker
*[AmountCalculator.sol#L18](https://github.com/1inch/limit-order-protocol/blob/fc528b390bad66927b9316470e4c86d06df58563/contracts/helpers/AmountCalculator.sol#L18)*

When filling an order with some specified `makingAmount`, the `takingAmount` was calculated with the `AmountCalculator.getTakerAmount()` function. This function calculates the ceiling amount to be taken from the order taker. As such, it was possible for additional assets to be taken from the taker beyond the order specification.

As an example, consider the order of trading 9000000 Dai for 7 Cards, being partially filled twice before being fully filled.

```
makerDai: 9000000, takerDai: 0
makerWeth: 0, takerCards: 10

~~~ fill some of the order ~~~

making amount: 2727273, taking amount: 3
makerDai: 6272727, takerDai: 2727273
makerCards: 3, takerCards: 7

~~~ fill some of the order ~~~

making amount: 818181, taking amount: 1
makerDai: 5454546, takerDai: 3545454
makerCards: 4, takerCards: 6

~~~ fill the rest of the order ~~~

making amount: 5454546, taking amount: 5
makerDai: 0, takerDai: 9000000
makerCards: 9, takerCards: 1
```

By the end of the order, the taker has received the full Dai amount, however, 9 Cards were taken from the taker instead of the specified 7. This issue relies on the taker having more of the specified taker asset than specified in the order, as well as contract approval to transfer the asset. Moreover, this relies on the taker partially filling the order before completely filling the order, in order for the `AmountCalculator.getTakerAmount()` function to calculate the ceiling taker amounts.

While the impact of this would likely be low in the case of an ERC-20 token with several decimal places, the risk is increased in the case of ERC-1155 tokens, which are much more likely to have a relatively low trade size and could easily lead to a relatively large error in the trade value.

#### Recommendation

The impact of the rounding issue should be further investigated by the 1inch team to determine the desired behavior. Either the taker or maker will have to be subject to rounding in some cases, so the risks should be well understood and communicated to users.

#### Update
The 1inch team indicated that this was the desired behavior.

## 5.3 Low Risk

No low risk issues were present at the conclusion of the audit.

## 5.4 Informational

### 5.4.1 Design comments

No informational issues were present at the conclusion of the audit.

## 5.5 Closed

### 5.5.1 `arbitraryStaticCall()` potentially vulnerable to front-running (high risk)
*[AmountCalculator.sol#L21](https://github.com/1inch/limit-order-protocol/blob/fc528b390bad66927b9316470e4c86d06df58563/contracts/helpers/AmountCalculator.sol#L21)*

Order makers are able to use the `AmountCalculator.arbitraryStaticCall()` function to specify their own maker and taker pricing functions. However, since the functions can exist in a user specified contract and function, it would be possible for a malicious maker to front-run a taker taking their order.

As an example, consider the following price function in their own malicious contract:

```solidity
function getTakerAmount(){
   if(!activated){
       return validAmount;
   }
   else {
       return 1;
   }
}
```

If calling the function, the user would see `validAmount` as the taker amount before filling the order. After submitting the transaction the maker would then be able to front-run the transaction to activate the malicious function, which would only return 1 unit.

#### Recommendation

Instead of allowing makers to specify their own pricing functions, it is recommended that a contract with popular pricing functions is deployed to restrict makers to only using functions within that contract. Alternatively, it may be necessary to remove the ability for makers to specify their own pricing algorithms.

## Update

[d72efda](https://github.com/1inch/limit-order-protocol/pull/14/commits/d72efdac9a2da5d138684081531eca5b5f753225) and [7a6c1df](https://github.com/1inch/limit-order-protocol/commit/7a6c1dfebd8d4615c53009a95990ddaefd9e1bcc) introduced further validation and allowed the caller to specify a `thresholdAmount` parameter to specify bounds for the trade succeeding.