We audited the changes made to:
In scope were the following contracts:
1inch-contract/contracts/
├── AggregationRouterV6.sol
routers/
├── ClipperRouter.sol
├── GenericRouter.sol
└── UnoswapRouter.sol
limit-order-protocol/contracts/
├── LimitOrderProtocol.sol
└── OrderMixin.sol
The 1inch team incorporated the following changes:
The LimitOrderProtocol contract now leverages the Ownable and Pausable libraries from the OpenZeppelin contracts suite (version 5.0.1) in order to:
owner) that can perform the aforementioned actions in case of an emergency and renounce ownership if necessary after pausing the contract if the team decides the contract should be deprecated.Note that only order filling can be effectively paused, as order cancellation, transaction simulation, and the permitAndCall entry point from the solidity-utils package (v3.7.1) will remain usable even if the contract is paused.
The Pausable library has also been added to the AggregationProtocolV6 contract. As such, the old mechanism of using selfdestruct to deprecate the contract in case of an emergency has been substituted with simply pausing the contract (and potentially renouncing ownership of the contract if necessary). The rationale behind this change is to plan for the future when the selfdestruct opcode gets discontinued.
All trading activity gets halted if the contract gets paused, except three entry points:
permitAndCall function inherited from the PermitAndCall libraryMore information about the consequences and trade-offs of leaving these two callbacks callable during a paused contract state can be found in the next section.
Regarding the three Limit Order protocol entry points that have been left callable during a paused state, the team confirmed this is a conscious decision which enables users to cancel outstanding orders in unorthodox scenarios where the contract needs to be paused. Transaction simulation does not pose security risks for the system and the PermitAndCall library is not helpful if it cannot be chained with another call to fill an order.
However, if the AggregationRouterV6 contract is paused, presumably due to an exploit or bug, the ERC-20 balances can still be transferred out by anyone despite the contract being paused. This can be achieved by leveraging either the curveSwapCallback or the uniswapV3SwapCallback functions.
The first function directly transfers arbitrary balances to arbitrary recipients, should there be any positive balance in the contract. The second one will revert if called in such a way so that the payer is not the AggregationProtocolV6 contract itself. However, if the payer is set to the contract address itself, it will go ahead and transfer arbitrary token balances to arbitrary recipients as long as the caller is a contract that implements the token0() and token1() functions.
In an emergency scenario (e.g., user funds being stuck on the contract), there is already an onlyOwner function that allows for the safe recovery of those funds, so no publicly accessible function should be available. Moreover, even if the contract is paused and ownership is renounced, malicious third parties may still use this contract to fund exploits on other DeFi protocols. While this in itself is not 1inch's fault, end users will be able to see this contract as the source of funds for the attack and may have negative PR effects for the company. Although, it is worth noting that the same attack can be achieved even when the contract is not paused.
Adding the whenNotPaused modifier to both callbacks could mitigate this situation in case the perceived risk of such scenarios outweighs the extra gas consumption for legitimate swaps on Curve pools with callbacks and Uniswap V3 while the contract is unpaused. Since the storage slot will be considered warm, an approximate extra 100 units of gas per swap involving callbacks will be used for every redundant whenNotPaused modifier used in legitimate trades. However, we did not find a strong reason to raise this as an issue since the same behaviour could happen while the contract remains unpaused.
In cases of emergency when the contracts are paused, users should immediately cancel all outstanding orders and revoke prior approvals as a safety measure.
Named return variables are a way to declare variables that are meant to be used within a function's body for the purpose of being returned as the function's output. They are an alternative to explicit in-line return statements.
Throughout the codebase, there are multiple instances of unused named return variables:
returnAmount return variable in the clipperSwap function in ClipperRouter.solreturnAmount return variable in the clipperSwapTo function in ClipperRouter.solConsider either using or removing any unused named return variables.
Update: Acknowledged, will resolve. The 1inch team stated:
We will make an effort to unify our usage of named return variable across all our active codebase somewhen later this year.
The audited version of both 1inch Limit Order Protocol and Aggregation Protocol allows the 1inch team to pause and unpause the main contracts and potentially deprecate them by renouncing ownership in case of an emergency.
No major issues were found during the audit, although some insights about the consequences of the changes introduced were added to the introduction. This was done to make both the client and end-users more aware of the full implications of the decisions behind the implemented changes.
The 1inch 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.