We audited the 1inch/limit-order-protocol repository at the 2f87361 commit and the 1inch/limit-order-settlement repository at the 1405320 commit.
In scope were the following files:
limit-order-protocol@2f87361
└── contracts
└── FeeTaker.sol
limit-order-settlement@1405320
└── contracts
├── Settlement.sol
├── SimpleSettlement.sol
└── extensions
├── BaseExtension.sol
├── ExtensionLib.sol
├── IntegratorFeeExtension.sol
├── ResolverFeeExtension.sol
└── WhitelistExtension.sol
This audit largely covers a refactor of code that had previously been audited by OpenZeppelin. What was SettlementExtension.sol at commit ff7909c has now been split into several different files. This refactoring allows modularity for developers to build their own extension contracts using only some of the features from what was a monolithic contract before. The different feature sets have been broken up as follows:
| Contract | Features |
|---|---|
BaseExtension |
creates empty, overridable pre and post interaction methods along with the IAmountGetter methods for fusion orders |
IntegratorFeeExtension |
allows an integrator to charge a percentage of the taking amount from the resolver |
ResolverFeeExtension |
charges the resolver fees from the resolver |
WhitelistExtension |
ensures that only whitelisted resolvers can settle fusion orders |
SimpleSettlement |
wraps all of the above into a single contract |
Settlement |
adds a check for a valid priority fee before any of the other extension logic |
ExtensionLib |
library for parsing data from the bitmask that is the last byte in the extraData parameter |
These contracts are meant to work with 1inch's Fusion system, where users' swaps are a dutch auction with an exchange rate that increasingly becomes better for the taker over time. A new feature added in this scope is that gas price increases can be factored in the dutch auction prices.
A new feeTaker contract has also been added to the core protocol that allows a third party intermediary to take fees in the taking tokens and transfers the rest to the maker.
The system relies heavily on the accuracy of off-chain calculation and signing. In order to handle such extensible workflows and yet be gas efficient, untyped bytes are often parsed and passed between contracts and functions. This requires not only the dapp to be incredibly careful when constructing order calls but also requires makers to be diligent with what they are signing. There are a lot of opportunities for functions to behave very different than intended. For example, passing the wrong auction rates and times to the extension could create an auction where things get more expensive over time instead of less expensive as intended. Because makers have to sign the data that they want to be executed on-chain, we assume that these users will do so accurately and with knowledge of how their orders will be executed. As for privileged roles within the scoped system, there is only one: the limit order protocol is the only allowed caller of the pre- and post-interaction functions of the Fusion system.
Throughout the codebase, docstrings are missing at some places:
The _getAuctionBump function declaration within BaseExtension.sol
Consider thoroughly documenting all functions (and their parameters) that are part of any contract's public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved in pull request #311 (commit 76d0640) and pull request #153 (commit 2eef6f8).
In WhitelistExtension's _isWhitelisted function resolvers are valid only after specific points in time and not before. This is clearly and competently written in the workings of the function. But that this is intended design is not described in the description of the function and led to some confusion on our part. Consider elucidating this detail in the natspec comments of the function so that further readers know how this function is intended to behave.
Update: Resolved in pull request #153 at commit b599e1d.
To make locating contract elements easier by using a common layout, the Solidity Style Guide recommends adhering to the following order of elements within each contract:
In the BaseExtension contract, private functions are declared before internal functions. This deviation from the commonly used layout may impact readability.
Consider moving the internal functions before the private functions.
Update: Resolved in pull request #153 at commit 51ed3ba.
This audit largely covered the refactoring of a contract in the limit-order-settlement repository into multiple contracts. This change added modularity and flexibility to the codebase. Additionally, a new extension contract was added to the limit-order-protocol.
No major issues were found during the audit, although some best practice recommendations were made in order to make the codebase even more robust.
The 1inch team was very responsive throughout the engagement, providing us with the necessary insight to expedite our understanding of the implemented changes.