We audited the 1inch/cross-chain-swap repository at commit 5158cb8.
In scope were the following files:
./contracts/
├── Escrow.sol
├── EscrowDst.sol
├── EscrowFactory.sol
├── EscrowSrc.sol
├── interfaces
│ ├── IEscrow.sol
│ ├── IEscrowFactory.sol
│ └── IEscrowSrc.sol
├── libraries
│ ├── Clones.sol
│ ├── ImmutablesLib.sol
│ └── TimelocksLib.sol
The Cross-Chain Swap Protocol allows users to swap ERC-20 tokens from a certain source chain with ERC-20 tokens (or the native tokens) at a different destination chain. The basic operating principle is that funds are escrowed on both chains and then are released after a certain period of time when a "secret" is revealed. This secret is the pre-image of a keccak256 hash, which is stored in each escrow contract as a "hashlock".
A set of privileged users called "resolvers" are able to obtain the secret during a specific time window, after which it is possible to release the destination chain funds to the user (maker) and the source funds to the resolver (taker). The resolvers must be whitelisted, which entails going through a KYC/KYB process and having an active stake in the 1inch system. This whitelisting setup incentivizes prompt and well-behaved cross-chain order filling from said users.
Secrets are generated and encrypted by the frontend and then the relayer will forward them to various resolvers. At first, the secret can only be utilised by the selected resolver (winner of the auction) who has successfully created and funded the escrow contracts on both chains. This resolver is responsible for finalizing the order. If this resolver fails to fulfill the order, then eventually a public withdraw phase is entered. In this phase, any other user or resolver holding the secret (which will be disclosed by the relayer to all resolvers or read on-chain if the original resolver completes one leg of the trade) can finalize the destination side of the swap. This means that the end-user receives their tokens from the swap.
Only the chosen resolver may withdraw tokens from the source side of the swap, but it is assumed that resolvers are technically competent and have a high level of liveness and ability to send transactions. However, there is a time window in which anyone can cancel the source escrow, releasing the escrowed funds back to the original user (maker). The taker is also able to rescue funds accidentally sent to both contracts after a delay which should be enough to complete or cancel the swap on both chains.
To encourage the swap to be finalized on both chains in a timely manner, a "security deposit" of native tokens is included on each chain. Once the swap is finalized or cancelled, the security deposit is transferred to the msg.sender to compensate them for gas expenses and incentivize them to move the trade forward. If the secret is never broadcast, the swap will eventually time out, after which both sides can be cancelled and the funds will be returned to their original owners.
There are two main privileged roles in the protocol:
The relayer: An off-chain component developed and maintained by 1inch. It manages the secret disclosure to the relevant on-chain parties. This actor holds all current, encrypted secrets for all orders and also has the power to disclose them incorrectly or hold them for a longer time than expected (effectively causing a DoS on the protocol) if it becomes compromised. Security around this component is paramount for the overall safety of the system.
Resolvers: Winners of the auctions will be able to act as takers of the cross-chain order and have the responsibility to finalize the order on both chains. They are expected to act diligently, promptly, and with liveness regarding the time windows created for the swap.
The protocol contains several trust assumptions, especially revolving around both privileged roles.
Regarding the relayer:
dstEscrow contract has been deployed after the srcEscrow contract, with the correct Immutables data and the correct safety deposit amount, before ever releasing the secret to the resolver. Otherwise, it is possible for a resolver to deploy a dstEscrow contract using arbitrary Timelock values that are against the maker's best interests. For instance, all time-deltas could be set to zero to ensure that the escrow can only be cancelled by the resolver himself with no risk of losing the funds or the safety deposit.Regarding all resolvers within the system:
Specifically for resolvers who win the auction:
Specifically for other resolvers not selected to fill a specific trade:
Regarding general protocol operations:
DeployedAt + <stage> never exceeds uint32(max), which corresponds to a timestamp in the year 2105.limit-order-protocol and limit-order-settlement protocols is correct as documented.EscrowFactory._postInteraction function within the audited repository.< indicates before < after: SrcWithdrawal < DstWithdrawal < DstPublicWithdrawal < DstCancellation <= SrcCancellation < SrcPublicCancellation.RESCUE_DELAY is significantly larger than the delta for SrcPublicCancellation and DstCancellation.It is also assumed that this protocol will only be used on chains which have enough decentralization to avoid reorgs which may reverse fund movements for these swaps. It is assumed that 1inch will wait for enough confirmations, on a chain-by-chain basis, before confirming the existence of swap contracts and releasing secrets to finalize the swap. It is assumed that the whitelist will be actively managed by 1inch and resolvers who misbehave will be immediately removed from the whitelist to prevent further abuse.
Aside from concrete recommendations made in the section below, there are general practices that can be followed to ensure that system assumptions are upheld and quickly detect and prevent abuse or misuse of the system by the privileged roles.
Within the user interface:
During secret creation:
random() function as it is simplistic, and avoid psuedorandom functions which involve a seed that could be guessed.bytes32 would narrow the set of potential secrets, making them easier to guess.During order creation:
timelocks for the "dst" escrow. The rules should also define the time window in which a valid "dst" escrow contract can be deployed.EscrowFactory is deployed for sequencer or block producer liveness. If there is a consensus failure or excessive delays in block production, do not allow new orders to be created for a pre-set period of time.During order filling:
EscrowFactory.createDstEscrow and parse the immutables used for creating these contracts. Immediately flag any deployed "dst" escrow contracts as invalid if they violate any of the rules defined in the "During order creation" section above.DstImmutablesComplement defined when creating the "src" escrow contract.After secret release:
rescueFunds. Check that these calls only transfer funds which were accidentally transferred to some escrow contract. In rare cases, it is possible that the rescueFunds function could be used in the "src" escrow to take funds which should have been transferred to the maker during the cancellation phase.Throughout the codebase, there are multiple instances of code that do not have docstrings or use incorrect NatSpec syntax:
RESCUE_DELAY state variable in Escrow.solFACTORY state variable in Escrow.solPROXY_BYTECODE_HASH state variable in Escrow.solESCROW_SRC_IMPLEMENTATION state variable in EscrowFactory.solESCROW_DST_IMPLEMENTATION state variable in EscrowFactory.solRESCUE_DELAY function in IEscrow.solFACTORY function in IEscrow.solPROXY_BYTECODE_HASH function in IEscrow.solCrosschainSwap event in IEscrowFactory.solImmutablesLib library in ImmutablesLib.solConsider 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 #54 at commit cf9bee6.
Throughout the codebase, there are several instances of incomplete docstrings:
The rescueFunds, withdraw, and cancel functions in IEscrow.sol do not have the immutables parameter documented.
The publicWithdraw function in IEscrowDst.sol do not have the secret and immutables parameters documented.
The withdrawTo function in IEscrowSrc.sol does not have target and immutables parameters documented. The publicCancel function within the same interface also does not have the immutables parameter documented.
Consider thoroughly documenting all functions/events (and their parameters or return values) that are part of a contract's public API. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved in pull request #54 at commit 49e31a8.
The cancel function within the EscrowDst contract can only be called by the taker who will be the recipient of both the safety deposit and the escrowed tokens.
Consider enforcing consistency and making the function a bit more gas-efficient by transferring the escrowed tokens to msg.sender instead of retrieving the taker address from the immutables data structure.
Update: Acknowledged, not resolved. The 1inch team stated:
After discussions within the team, we decided not to fix this issue. The difference in gas is close to 0 (our tests showed ~3-4), with readability, it does not get any better.
When a cross-chain order has been filled on the source chain, the CrosschainSwap event is emitted. However, when the EscrowDst contract is deployed on the destination chain, no event is emitted.
Consider emitting an event upon deployment of the EscrowDst contract in order for off-chain logic to be able to properly track when both contracts are already deployed and the chronological order in which they were deployed.
Update: Resolved at commit 3de7020. The old CrosschainSwap event has been replaced by two new events (SrcEscrowCreated and DstEscrowCreated) which are emitted when each escrow contract is deployed in order to facilitate off-chain tracking. In addition, a new SecretRevealed event is being emitted upon public withdrawal including the secret.
Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled.
Throughout the codebase, there are multiple floating pragma directives:
Clones.sol has the solidity ^0.8.20 floating pragma directive.IEscrow.sol has the solidity ^0.8.0 floating pragma directive.IEscrowDst.sol has the solidity ^0.8.0 floating pragma directive.IEscrowFactory.sol has the solidity ^0.8.0 floating pragma directive.IEscrowSrc.sol has the solidity ^0.8.0 floating pragma directive.ImmutablesLib.sol has the solidity ^0.8.20 floating pragma directive.Consider using fixed pragma directives.
Update: Acknowledged, not resolved. The pragma directive in TimelocksLib has been changed to floating (^0.8.20) at commit f4b6f20 and the rest have remained unchanged. The 1inch team stated:
We prefer to keep the public interfaces and libraries with the floating pragma directive to provide flexibility when importing them into other projects.
publicWithdraw Can Happen During CancellationWithin the EscrowDst contract, the publicWithdraw function is callable at any time after the DstPublicWithdrawal timelock stage. This means that it could potentially be called after the escrow is supposed to be cancelled, thereby allowing a withdrawal at the destination chain and a cancellation at the source chain. This also mismatches with the comment on line 49 which explicitly states that the function is not callable during the cancellation phase.
Consider utilizing the onlyBetween modifier to prevent this function from being called after the cancellation phase begins.
Update: Resolved in pull request #53 at commit 13b7676. The modifier has been changed so that publicWithdraw can only be called between DstPublicWithdrawal and DstCancellation timestamps, effectively disallowing this call during the cancellation period.
The audited version of the 1inch Cross-Chain Swap Protocol enables users to seamlessly and gaslessly perform cross-chain swaps by leveraging the existing infrastructure from 1inch Fusion and the Limit Order Protocol. It provides a simple yet elegant way to incentivize whitelisted resolvers to promptly bridge and swap user assets. 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. There are strong trust assumptions reflected in the introduction about the off-chain component (the relayer) which should be studied carefully.
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, while being open to our best practices recommendations.