We audited the changes made to the 1inch/limit-order-protocol repository between the 4.0.0-prerelease-8 tag and the master branch at that time. The corresponding commits were e9b4573 and aab3364.
In scope were the following contracts:
contracts
├── LimitOrderProtocol.sol
├── OrderLib.sol
├── OrderMixin.sol
└── libraries
├── ExtensionLib.sol
├── MakerTraitsLib.sol
├── RemainingInvalidatorLib.sol
└── TakerTraitsLib.sol
The 1inch team incorporated the following changes into the protocol:
fillOrder, fillOrderArgs, fillContractOrder, fillContractOrderArgs. The first pair works with EOA signatures, and the second pair works with smart contract signatures that follow the ERC-1271 standard. The first and third functions work with simple orders while the second and fourth functions support taker interactions and maker extensions.NO_IMPROVE_RATE flag has been removed.permitAndCall convenience function to execute a permit and fill an order in one function call. Supported permits include ERC-2612 permits, DAI-like permits, and Permit2.IAmountGetter interface.not of the remaining amount instead of the remaining amount plus 1 wei. A value of zero means that the order is either a new one or does not exist. The value of type(uint256).max means that the order is either fully filled or cancelled.OrderLib.isValidExtension internal function now returns a boolean and an error code instead of reverting.The Limit Order Protocol implements an order book with makers creating and signing orders off-chain, and takers filling these orders on-chain. Makers can expose their orders to takers via the Limit Orders API or by any other means. The protocol relies on ERC-2612 and DAI-like permits for EOA signatures, EIP-712 and ERC-1271 for smart contract signatures, and also supports Permit2. Additionally, it comes with plenty of extensions that can be enabled to customize orders.
The LimitOrderProtocol contract allows the filling and cancelling of orders along with helper functionality for takers. It is possible to have orders with maker extensions and taker interactions or without them. Furthermore, the maker can be an EOA or a contract. Thus, there are four different functions to fill an order.
Order cancellation comes in three flavours depending on the order parameters:
Basic orders which do not have any extensions support the following features:
Extensions introduce the following features:
Additionally, the taker can use taker traits for the following features:
The following security considerations are not inherently bugs or vulnerabilities but may lead to them if not accounted for.
We found the codebase to be very well designed, with abundant documentation and highly optimized for gas efficiency. It also offers a great variety of settings to both takers and makers. We are glad to see robust security patterns in place and thoughtful considerations being applied to follow the best practices.
Users have two options to cancel their orders:
cancelOrder function, which uses both the BitInvalidator and the RemaningInvalidator approaches for cancellation, and emits the OrderCancelled event.bitsInvalidateForOrder function, which uses only the BitInvalidator approach and does not emit any event regarding order cancellation.Given that the BitInvalidator approach invalidates multiple orders at once and is not bound to a specific order hash, it is natural to skip emitting the OrderCancelled event within it. However, when using this approach via the cancelOrder function, the event will be emitted nonetheless. Furthermore, a malicious user may leverage this behaviour to confuse off-chain systems by emitting the OrderCancelled event for order hashes that are not actually cancelled.
Consider not emitting the OrderCancelled event for BitInvalidator cancellations within the cancelOrder function, and instead emitting another event that contains more relevant information. Moreover, consider refactoring the cancelOrder and bitsInvalidateForOrder functions to separate BitInvalidator and RemaningInvalidator approaches. This will also protect users from using incorrect maker traits with the help of the OrderIsNotSuitableForMassInvalidation error.
Update: Resolved in pull request #292 at commit 29199d8. The 1inch team added a new BitInvalidatorUpdated event, which is triggered when orders with useBitInvalidator are cancelled. Normal cancellations by orderHash still trigger the old OrderCancelled event.
makerPermit ExtensionMakers have the option of giving allowance via permit, in which case the HAS_EXTENSION_FLAG from the MarketTraitsLib library would need to be set and the extension should contain the makerPermit payload.
When takers fill such an order, if no prior allowance exists, the SKIP_ORDER_PERMIT_FLAG needs to be set to zero in order to ensure the permit will be consumed right after verifying the signature.
In order to consume the permit, both the target address and the permit payload need to be extracted from the extension bytes. The first 20 bytes of the maker permit will be used as the target address, while the remaining bytes will be used as the payload.
Depending on the permit length, the call will be managed differently, always respecting the first 20 bytes of the makerPermit as the address to call, except when the payload indicates that it is a Permit2, in which case the proper contract will be called.
Malicious makers can inject a malicious makerPermit bytes parameter encoded within the order extension so that the tryPermit function calls a contract owned by them. When called, this contract can perform arbitrary actions at the expense of the taker, since they are the ones funding the gas fees. Potential gas-heavy actions that could be run for free include:
This attack will be possible as long as:
In order to mitigate this attack vector, consider disabling makers from being able to specify the target address within the makerPermit bytes. If makers want to use Permit2, the SafeERC20 library is already overriding the target value. For ERC-2612-compatible permits, or exotic permit structures such as with the DAI token, the target address should be the makerAsset. This will also help optimize gas consumption when using permits since extensions will use 20 bytes less of calldata.
Even though takers will still make a profit and this technique could be considered within the "rules of the game", consider following this recommendation in order to make the service fairer to all takers while also making the protocol slightly more gas efficient.
Update: Acknowledged, not resolved. The 1inch team stated:
We can't be sure that
makerAssetis actually the token. When using proxies, for example,makerAssetis the address of the proxy and not the asset itself. And also, in that case,makersets the allowance to the proxy address, not to theLimitOrderProtocoladdress.
Gas limits on calls are considered a bad practice for the following reasons:
These factors could lead to unreliable functionality over time and across chains. A smart contract wallet can have logic in its receive or fallback function which exceeds the 5000 gas limit, thereby disabling that wallet to use the limit order protocol for swaps where the native currency is involved.
The following calls have a hard-coded gas limit:
msg.sender.call{value: msg.value - takingAmount, gas: _RAW_CALL_GAS_LIMIT}("") in the contract OrderMixin in OrderMixin.solorder.getReceiver().call{value: takingAmount, gas: _RAW_CALL_GAS_LIMIT}("") in the contract OrderMixin in OrderMixin.solTo ensure expected behavior and prevent external calls from running out of gas, consider removing the gas limitation or providing an explicit reason in the documentation for such limit.
Update: Resolved in pull request #292 at commit fcd4732. The 1inch team decided to remove the hard-coded gas limit on both external calls.
The function permitAndCall in the OrderMixin contract is one of the main entry points for takers when filling orders if they want to execute a permit before filling an order. However, it is not part of the IOrderMixin interface.
Consider adding this function to the IOrderMixin interface in order to have a comprehensive API for third parties looking to integrate with the protocol.
Update: Resolved in pull request #292 at commit 1800be9.
Named return variables are used in a function's body to be returned as the function's output. They are an alternative to explicit in-line return statements.
In ExtensionLib.sol, the result return variable for the _get function is unused.
Consider either using or removing any unused named return variables.
Update: Resolved in pull request #292 at commit 48c2cc1.
In the OrderMixin contract, the _PERMIT2 state variable is unused.
To improve the overall clarity, intentionality, and readability of the codebase, consider removing any unused state variables.
Update: Resolved in pull request #292 at commit 69a7a08.
Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of a mapping's purpose.
Throughout the codebase, there are multiple mappings without named parameters:
_bitInvalidator state variable in the OrderMixin contract_remainingInvalidator state variable in the OrderMixin contractConsider adding named parameters to the mappings to improve the readability and maintainability of the codebase. Particularly, the _remainingInvalidator mapping could benefit from being more explicit about the key names.
Update: Resolved in pull request #292 at commit 154519f.
Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice proves beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. Furthermore, if the contract incorporates third-party libraries and a bug surfaces in these, it becomes easier for the maintainers of those libraries to contact the appropriate person about the problem and provide mitigation instructions.
Throughout the codebase, there are contracts that do not have a security contact:
ExtensionLib contractLimitOrderProtocol contractMakerTraitsLib contractOrderLib contractOrderMixin contractRemainingInvalidatorLib contractTakerTraitsLib contractConsider adding a NatSpec comment containing a security contact above the contract definition. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.
Update: Acknowledged, will resolve. The 1inch team stated that they will follow this practice in the future.
The permitAndCall function of the OrderMixin contract uses the version of tryPermit with four parameters. However, there is a shorter version of tryPermit which matches the use case.
Consider using the shorter version for conciseness.
Update: Resolved in pull request #292 at commit c83322c.
Throughout the codebase, there are several parts that do not have docstrings:
TakerTraitsLib.sol claim that four bits are used for flags and the remaining bits are used to store the threshold amount, whereas the threshold amount actually takes fewer bits. The same docstrings in TakerTraitsLib.sol are missing documentation for the _ARGS_HAS_TARGET flag, the _ARGS_EXTENSION_LENGTH_OFFSET and _ARGS_INTERACTION_LENGTH_OFFSET values, and bits from 250 to 248.MakerTraitsLib.sol are missing documentation for the 253rd bit. While the bit is related to a removed feature and is not used anymore, this fact should nonetheless be stated explicitly.OrderMixin.sol claim that the function processes taker interaction while it also processes maker extensions. The same docstrings on line 461 in OrderMixin.sol claim to revert if the taker permit is invalid, whereas permits are not checked to begin with.OrderMixin.sol are missing.predicate2 twice on line 479 instead of mentioning predicate2 and predicate3 once.IOrderMixin.sol refer to the old way of storing the remaining amount.description.md file has not been updated to reflect the new changes.Consider updating the documentation so that it reflects all the newly introduced changes.
Update: Resolved in pull request #285 at commits b21a2e and ca2158d, and in pull request #292 at commits 1800be9, aa88e62, 46f4795 and 3c9b8ab.
The audited version of the 1inch Limit Order Protocol enables highly efficient permissionless swaps between makers and takers. It provides a high degree of flexibility and customizability while ensuring safety for all parties involved. We found the codebase to be very well-written, thoroughly documented, and optimized for gas consumption.
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 team was very responsive throughout the engagement, providing us with the necessary insight to expedite our understanding of the changes implemented and their effect on the codebase.