دسامبر 16، 2021
معرفی
La 1inch team asked us to review and audit their پروتکل سفارش محدود v2 smart contracts. We looked at the code and now publish our results.
حوزه
ما تعهد را حسابرسی کردیم 4d94eea25e4dac6271bfd703096a5c4a4d899b4a
از 1inch/limit-order-protocol
repository. In scope were the following contracts:
- OrderMixin.sol
- OrderRFQMixin.sol
- PredicateHelper.sol
- RevertReasonParser.sol
- Permitable.sol
- ChainlinkCalculator.sol
- ArgumentsDecoder.sol
- AmountCalculator.sol
- NonceManager.sol
- LimitOrderProtocol.sol
- ImmutableOwner.sol
- InteractiveNotificationReceiver.sol
- AggregatorInterface.sol
- IDaiLikePermit.sol
All other project files and directories (including tests), along with external dependencies and projects, game theory, and incentive design, were also excluded from the scope of this audit. External code and contract dependencies were assumed to work as documented and back-end services provided by 1inch were assumed to act in the best interest of the protocol.
سلامت کلی
In general, we found the project’s codebase to be readable and well organized, although it could benefit from more extensive documentation, especially around the blocks of assembly code, edge cases of the protocol, assets/predicates/external-resources that will be used, responsibilities/limitations of the back-end service provided, and interactions between actors. The project goes to great lengths to make actions gas-efficient, occasionally even at the risk of making the code more difficult to reason about; we raise issues related to that below. Throughout the audit, the 1inch team was highly available, responsive, and very easy to work with.
بررسی اجمالی سیستم
The Limit Order Protocol enables order makers
to sign orders off-chain for token swaps. The protocol then facilitates the filling of previously signed orders by order takers
. Orders are highly extensible, and can static-call external contracts at several points throughout the order filling process. This extensibility imbues the protocol with utility, but it adds both complexity and a greater attack surface for the orders themselves.
It is important to remark that there is no on-chain storage of order details. The fill-state or cancellation status of orders is only tracked via order hashes. This necessitates that orders be shared peer-to-peer or via a centralized party. In this case, the 1inch team intends to act as that centralized party, aggregating signed orders and using those orders as a source of liquidity for their other protocols. Orders will be published via their own API so users are able to interact with them.
This centralization gives the 1inch team extreme control over which orders are published and ultimately executed. This also gives them the ability to censor orders, which could be useful in the case of malicious or deceptive orders, but could also be abused and allow them to frontrun any other user in case of a favorable order by not showing it through the API.
نقش های ممتاز
Though the contracts the role is used in were out of scope, one privileged role was identified. An immutableOwner
is set to the creator of a proxy contract at the time of construction, and is used to limit access to the proxy’s external
توابع.
External dependencies and trust assumptions
The design of this protocol necessitates off-chain and on-chain components, and this hybrid model can be used to mitigate some attack vectors we identify in our report, but the cost of that ability is increased reliance on the 1inch team and infrastructure.
Additionally, the Limit Order Protocol provides functions that are meant to retrieve prices from Chainlink oracles. We assumed those oracles to be honest, accessible, and properly functioning.
Moreover, due to the flexibility of an order, there are several points of contact with external contracts which are not validated. This means that a malicious user could abuse such calls and impersonate predicates, assets, or oracles with malicious contracts in order to execute actions during order fills. Although the project is protected in some areas against reentrancy, such vectors could cause denial of service attacks or undetected spam orders. The 1inch team is aware that certain issues may appear when using unfamiliar contracts for the protocol and has indicated their intent that only major “blue chip” assets will be fully supported by the project. However, it should be noted that even with the most popular assets there are intrinsic behaviors from each asset that may cause problems on protocols that do not address them properly, such as having a fee during transfers with USDT or returning an error code instead of a success boolean with cTokens.
یافته ها
Here we present our findings.
شدت بحرانی
ندارد.
شدت بالا
[H01] Inconsistent data passed into _makeCall
در OrderMixin
قرارداد، _makeCall
function is used to transfer assets from the taker to the maker و پس از آن from the maker to the taker. In the latter transfer, the _makeCall
function is incorrectly passed the order’s makerAsset
as the last parameter, when it should be the order’s makerAssetData
.
As a result, any proxy functionality that relies on the makerAssetData
argument will break.
To be consistent with the earlier call to _makeCall
and to fully support proxy functionality, consider updating the order.makerAsset
پارامتر به order.makerAssetData
.
به روز رسانی: ثابت در درخواست کشش شماره 57.
[H02] Partially-filled private orders can be filled by anyone
The protocol allows the creation of private and public orders. On private orders, only the allowedSender
address, specified by the maker during the order’s creation, is able to fill the order.
با این حال ، در OrderMixin
قرارداد، validation for the allowedSender
نشانی is incorrectly scoped, meaning that it is only evaluated inside the logic that handles the first fill of an order. If a private order is partially filled, then the check for the allowedSender
address is no longer reachable and the order becomes fillable by anyone.
To clarify intent around whether any user should be able to fill partially-filled private orders or not, consider either documenting the reason for the current behavior or validating the allowedSender
address outside of the scope of the first fill to ensure that it will be validated every time a fill is attempted.
به روز رسانی: ثابت در درخواست کشش شماره 58.
[H03] Malicious maker could take advantage of partial fills to steal taker’s assets
Orders from the OrderMixin
contract have the ability to be partially filled. To support partial fills, the protocol requires a way to calculate both sides of swaps. Both getMakerAmount
و getTakerAmount
fields are defined by the maker of the order for this exact purpose.
When filling an order, takers must provide either the makingAmount
یا takingAmount
values as well as a thresholdAmount
value. There are two different code-paths that can be taken, based on if the makingAmount
یا takingAmount
تهیه شده بود.
The first one is when the makingAmount
parameter is defined. It could کوتاه کردن la makingAmount
value and also محاسبه کنید takingAmount
value for it. In this situation, the thresholdAmount
تضمین می کند که takingAmount
value taken is not unexpectedly large.
The second one is when the takingAmount
parameter is defined. In such case, it will محاسبه کنید makingAmount
value, with the possibility of truncating it و recalculating the takingAmount
value if that happens. In this situation, the thresholdAmount
value ensures that the makingAmount
value returned is not unexpectedly small.
There exist two exploitation methods, each unique to one of the previous mentioned code-paths. These exploitation methods require malicious getMakerAmount
و getTakerAmount
functions. A simple implementation of these functions would have an identical behavior to AmountCalculator
's getMakerAmount
و getTakerAmount
functions, but with a hard-coded switch that will force them to return an attacker controlled value when needed.
The first, less severe exploit pattern involves the first code-path where the makingAmount
value is specified in a fill order. A malicious maker would wait for a fill order which specifies makingAmount
to show up in the mempool in order to frontrun it. They would drain all of the value except 1 from the maker’s side and then force _callGetTakerAmount
to return the amount specified in the user’s thresholdAmount
value (or their allowance if it is less). When the user’s transaction finally goes through, they will swap their full thresholdAmount
ارزش takerAsset
for a single unit of makerAsset
. This exploit is limited by the amount given by the thresholdAmount
value or the amount of the takerAsset
the user allowed on the LimitOrderProtocol
قرارداد.
The second, more severe exploit pattern involves the second code-path where the takingAmount
value is specified. The malicious maker would similarly wait for a fill order that specified a takingAmount
value to show up in the mempool. They would frontrun the transaction and force the makingAmount
مقدار بازگشتی توسط _callGetMakerAmount
function to be higher than both the remainingMakerAmount
و thresholdAmount
. They would also set the takingAmount
returned value by _callGetTakerAmount
to be the amount of takerAsset
asset allowed on the LimitOrderProtocol
by the taker. When the taker’s transaction goes through, it will truncate the makingAmount
value and then recalculate the takingAmount
value. This recalculation is not guaranteed to be lower however, and in this case will drain the taker of all the takerAsset
that they allowed on the contract. In this code-path, the thresholdAmount
ارزش است اطمینان از اینکه makingAmount
is not too low, so taking all the taker’s takerAsset
asset is unchecked. The funds lost are bounded by the amount of the takerAsset
asset the user allowed on the LimitOrderProtocol
قرارداد.
These exploits are not possible without partial orders and, more specifically, partial orders with malicious getMakerAmount
و getTakerAmount
پیاده سازی
The main issue of the thresholdAmount
value check is that it only covers one side of the swap, but the other side can be manipulated via frontrunning. There are no assurances that the value the taker originally proposed remains unchanged. Consider removing makingAmount
truncation from both code-paths and reverting if the order cannot support a fill as large as requested. By doing this, the thresholdAmount
can be used to sufficiently restrict the other side of the swap and avoid unexpected behavior, even in malicious orders.
به روز رسانی: ثابت در درخواست کشش شماره 83.
شدت متوسط
[M01] Static arguments passed after dynamic arguments
در OrderMixin
قرارداد، getTakerAmount
و getMakerAmount
bytes fields are used as arguments for the _callGetTakerAmount
و _callGetMakerAmount
functions. These calls provide a way to calculate one side of the swap based on the other side, and they allow users to partially fill orders.
La getTakerAmount
/getMakerAmount
fields are dynamic variables and are packed in front of the takerAmount
و makerAmount
مقادیر در _callGetTakerAmount
و _callGetMakerAmount
functions. It is possible for a malicious maker to provide more data than expected in the getTakerAmount
وgetMakerAmount
fields to push the takerAmount
و makerAmount
bytes past where they are assumed to be when being decoded in the next function. This allows the maker to shift the passed in taker or maker amount by a full bytes to the right and even replace them completely if an extra 32 bytes of data is provided.
Users already have to manually review the getTakerAmount
و getMakerAmount
fields in the order, but this technique is rather hard to spot. Also worth noting, this attack even applies to the internally trusted getMakerAmount
و getTakerAmount
functions. For most attacks, providing a reasonable threshold amount will prevent loss of funds.
To prevent this, consider encoding the static arguments before the dynamic arguments to avoid giving the dynamic arguments a method to control the static arguments.
به روز رسانی: Not fixed. The 1inch team stated:
We’ll take extra care with getters validation. We’ll try to implement sanity validation of getters in our sdk that will help with filtering potentially malicious orders.
[M02] ERC721 orders can be manipulated
It is possible to exchange more than just ERC20s via the OrderMixin
by deploying a contract that shares the same function selector as IERC20’s transferFrom
, and providing that contract as the makerAsset
یا takerAsset
in an order.
The out-of-scope proxies, namely, ERC721Proxy
, ERC721ProxySafe
و ERC1155Proxy
contracts follow this pattern to provide support for ERC721
و ERC1155
tokens. Since the proxies must be called with the same pattern as an IERC20 transferFrom
call, the signature must start with address from
, address to
و uint256 amount
. Anything else that the proxies require can be passed in after, and is defined in the order as makerAssetData
و takerAssetData
.
ERC1155s can naturally transfer multiple of the same id tokens at once, which means the ERC1155Proxy
contract makes use of the amount
field. On the other hand, ERC721
s do not have an obvious use for the amount
field. Since they represent non-fungible tokens, a specific tokenId will only have one in existence, rendering the amount
field useless. Because of this, the implementation for both ERC721Proxy
و ERC721ProxySafe
contracts use the required amount
field as the tokenId
به جای آن.
This overloading of the amount
parameter creates the possibility of partially filling ERC721
orders in order to purchase separately listed tokens at discounted prices. For instance, there could be a case where a single user has multiple ERC721
s of the same contract permitted to be transferred by the ERC721Proxy
contract and lists them in separate limit orders.
If the limit orders also provide the getMakerAmount
و getTakerAmount
fields, it will be possible to partially fill these ERC721
orders. Since the order’s amount
field actually corresponds to the tokenId
, a malicious user can place a partial fill on the ERC721
with the higher tokenId, resulting in a makingAmount
/takingAmount
از ERC721
that could correspond to a lower tokenId
. نتیجه این است ERC721
با پایین تر tokenId
would be transferred at the price of (higher tokenId price) * (lower tokenId's id) / (higher tokenId's id)
.
This exploit has a few requirements:
- چندین
ERC721
s from the same contract to be allowed on eitherERC721
proxy by a single owner. - Open order for one of the
ERC721
s that is not the lowesttokenId
of the ones allowed. - Partial fills allowed on the order.
To completely remove the possibility of partial ERC721
fills, consider separating the amount
و tokenId
arguments. Whether the arguments are separated or not, consider also documenting this to alert users of this behavior and to avoid this pattern in the future.
به روز رسانی: ثابت در درخواست کشش شماره 59.
[M03] Undocumented decimal assumptions
La LimitOrderProtocol
contract inherits the ChainlinkCalculator
contract through the OrderMixin
contract. This contract exposes two functions to enable the usage of Chainlink oracles during the predicates check and the lookup of the maker amount/taker amount.
However, the contract makes undocumented assumptions about the number of decimals that the Chainlink oracles should report in, as well as the number of decimals that the function parameters should contain. In certain scenarios, this could lead to unexpected behaviors, including the mis-pricing of assets and the unintentional loss of funds.
More specifically, throughout the contract the implicit assumption is that the Chainlink oracles will report with 18 decimals of precision. However, not all Chainlink oracles report with this number of decimals. In fact, if the oracle reports a token pair that is in terms of a currency (USD, for instance), it will only have 8 decimals of precision. Since there are no restrictions on که oracles can be used, implicit assumptions should not be made about the number of decimals they will report with.
Relatedly, there is an implicit assumption that the amount
پارامتر برای ChainlinkCalculator
functions will use 18 decimals, together with the misleading explicit declaration that the singlePrice
تابع Calculates price of token relative to ETH scaled by 1e18
. In reality, even with an oracle that میکند report with 18 decimals, the return value of the singlePrice
function would be scaled by the number of decimals of the amount
parameter, which may not necessarily be 18 decimals.
به طور مشابه، doublePrice
function assumes that two Chainlink oracles will report with the same number of decimals, causing the result of the function to deviate from expectations.
Consider explicitly documenting assumptions regarding the number of decimals that parameters and return values should be in terms of. Furthermore, consider either limiting calculations that depend on oracles that break those assumptions, or having the relevant calculations take the actual number of decimals into account.
به روز رسانی: ثابت در درخواست کشش شماره 75.
شدت کم
[L01] Constants not declared explicitly
There are a few occurrences of literal values being used with unexplained meaning in the codebase. For example:
- در
OrderMixin
قرارداد،_remaining
mapping is semantically overloaded (as explained in the issue Semantic overloading of mapping) to track the amount of asset remaining for a partially filled order همچنین if an order has been completely filled. Specifically,0
means that no fills associated with an order have been made,1
means an order can no longer be filled, and anything larger than1
means that there is a remaining amount associated with the order that can potentially be filled. - در
ChainlinkCalculator
contract, the literal value1e18
در استفاده می شودsinglePrice
تابع.
To improve the code’s readability and facilitate refactoring, consider defining a constant for every magic number, giving it a clear and self-explanatory name. For complex values, consider adding an inline comment explaining how they were calculated or why they were chosen.
به روز رسانی: ثابت در درخواست کشش شماره 75 و درخواست کشش شماره 76.
[L02] Malicious parties could prevent the execution of permitable orders
La OrderMixin
contract allows maker users to submit permitable orders so those can be executed in one transaction, rather than having to have a separate transaction for approvals. Also, order takers can submit their own permit during the filling of the order for the same purpose.
However, because the maker’s permit is contained inside the سفارش, both the maker’s and the taker’s permits would be accessible while the order-fill transaction is in the mempool. This would make it possible for any malicious user to take those permits and execute them on the respective asset contracts while frontrunning the fill transaction. Because these permits have a nonce
to prevent a double spending attack, the order’s fill transaction would fail as a result of trying to use the same permit that was just used during the frontrun.
Although there is no security risk, and the maker could create a new order and pre-approve the transaction, this attack could certainly impact the usability of permitable orders. Indeed, a motivated attacker could block تمام permitable orders with this attack. Consider validating if the permit was already submitted, or if the allowance is enough, during the order fills. Also consider letting users know about this possible attack during order composition.
به روز رسانی: Not fixed. The 1inch team states:
We had approval checks before but decided to simplify permit flow to just revert on unsuccessful approvals. We’ll think about the ways to notify makers about the issue.
[L03] Duplicated code
There are instances of duplicated code within the codebase. Duplicating code can lead to issues later in the development lifecycle and leaves the project more prone to the introduction of errors. Such errors can inadvertently be introduced when functionality changes are not replicated across all instances of code that should be identical. Examples of duplicated code include:
Rather than duplicating code, consider having just one contract or library containing the duplicated code and using it whenever the duplicated functionality is required.
به روز رسانی: تا حدی ثابت شده است درخواست کشش شماره 60.
[L04] Erroneous or misleading test suite
There are instances in the test suite where the tests deviate from their expected behavior. For instance:
- La
ChainlinkCalculator
contract is inherited by theOrderMixin
contract. However, during the tests theAmountCalculator.arbitraryStaticCall
function is used to call theChainlinkCalculator
contract as an external, independent contract. Even though the result is the one expected, the test should reflect the behavior with the current design of the system and anticipated use case by callingChainlinkCalculator
functions directly without using the arbitrary static call. - Though the proxy contracts were out of scope, we noticed that when testing the protocol with ERC721 assets, the
ERC721Proxy
contract is not used to swap the assets in its مجموعه تست.
As the test suite itself is outside the scope of this audit, please consider thoroughly reviewing the test suite to make sure all tests run successfully according to the specifications of the protocol.
به روز رسانی: ثابت در درخواست کشش شماره 57, درخواست کشش شماره 59و درخواست کشش شماره 61.
[L05] Errors and omissions in events
Throughout the codebase, events are generally emitted when sensitive changes are made to the contracts. However, many events lack indexed parameters and/or are missing important parameters. For example:
There are also sensitive actions that are lacking events, such as:
Consider more completely indexing existing events and adding new parameters where they are lacking. Also, consider emitting all events in such a complete manner that they could be used to rebuild the state of the contract by off-chain services.
به روز رسانی: Not fixed. However, the 1inch team did add an orderRemaining
پارامتر به OrderCanceled
رویداد در درخواست کشش شماره 62.
The 1inch team states:
We found that only a limited subset of data is required to satisfy frontend needs. In the case of extensive analysis, all the suggested fields are available via tracing. For
OrderRFQMixin
we expect market makers to build their own sophisticated way of tracking what orders have been canceled.
[L06] Storage changes during event emission
در NonceManager
contract, when the NonceIncreased
event is emitted, the nonce of the message sender is also increased.
Executing multiple operations simultaneously can make the codebase harder to reason about, more prone to errors, and can lead to operations being overlooked or misunderstood.
To improve the overall intentionality, readability, and clarity of the code, consider increasing the nonce value before emitting the event.
به روز رسانی: ثابت در درخواست کشش شماره 63.
[L07] Inconsistent decoding methodologies could cause outcome discrepancies
To support all of its extensibility and flexibility, the Limit Order Protocol routinely has to deal with dynamic bytes data and arbitrary return values from external contracts. As a result, the protocol includes an ArgumentsDecoder
library to more efficiently convert dynamic bytes values into basic data types. However, this library is not used exclusively, and in some cases abi.decode
is used instead. Additionally, some contracts are using abi coder v1
while others are using abi coder v2
. The former performs more similarly to the ArgumentsDecoder
library, whereas the latter performs additional checks when decoding.
The inconsistent usage of these different decoding methodologies can result in subtle discrepancies between the intention and actual behavior of the codebase.
به عنوان مثال، simulateCalls
function only uses the ArgumentsDecoder.decodeBool
عملکرد. اگر simulateCalls
function is used to check calls that would be made in the predicate part of an order, then its results could deviate from what actually occurs when the predicate conditions are evaluated, because different decoding methodologies are employed.
So, for instance, if a predicate makes an external staticcall
to some function that returns a uint256
value greater than one rather than the expected bool
, then that call will revert, because the return value is decoded with abi coder v2
's abi.decode
which will not accept such values as bool
. However, if the exact same call is made with simulateCalls
، پس از آن will simply be marked as true
، به دلیل decodeBool
treats any value larger than zero as true
.
برای ساختن simulateCalls
function fully mirror the behavior of actual predicate calls, consider modifying it to use abi.decode
.
به روز رسانی: ثابت در درخواست کشش شماره 82.
[L08] Lack of input validation
La fillOrderToWithPermit
و fillOrderTo
توابع از OrderMixin
contract, as well as the fillOrderRFQToWithPermit
و fillOrderRFQTo
توابع از OrderRFQMixin
contract, do not validate the target
address parameter.
This makes it possible for a user to inadvertently pass in the zero address and, as a result, lock up the assets they are meant to receive after filling an order.
To ensure that users do not accidentally lock up their funds, consider validating that the target
address does not equal the zero address in the cited functions.
به روز رسانی: ثابت در درخواست کشش شماره 78.
[L09] Low unit test coverage
The unit test coverage for the entire project is around 75%, with some of the contracts having particularly low coverage.
Considering the importance of unit tests to validate code and prevent regressions when refactoring and developing new features, we encourage significantly increasing unit test coverage to at least 95%, and including edge cases that cover even unlikely situations.
به روز رسانی: درست نشد.
[L10] Misleading or incomplete inline documentation
Throughout the codebase, a few instances of misleading and/or incomplete inline documentation were identified and should be fixed.
The following are instances of misleading inline documentation:
- در
ChainlinkCalculator
قرارداد،singlePrice
کارکرد NatSpec@notice
برچسب says that itCalculates price of token relative to ETH scaled by 1e18
, but in fact, its result is the ارزش ofamount
tokens scaled by1e18
, where the oracle may not report in terms of ETH (for a pair not including ETH, for instance). - در
OrderRFQMixin
قرارداد،invalidatorForOrderRFQ
کارکرد NatSpec@return
برچسب is misleading, because the quote may not have been filled for the respective invalidator bit to have been set. The order can also have been canceled. - On lines 147, 165و 188 of
OrderMixin.sol
, the NatSpec@param
tags are ungrammatical. - برخط 20 of
ERC1155Proxy.sol
از@notice
tag states that the computed hash is the result of hashing thefunc_733NCGU
function, where it should be thefunc_301JL5R
در عوض عملکرد
The following are instances of incomplete inline documentation:
- Functions in the
AmountCalculator
contract do not describe any of the parameters. - در
ChainlinkCalculator
قرارداد،singlePrice
وdoublePrice
functions do not describe all of the parameters. - در
ImmutableOwner
contract, the public variable and modifier have no NatSpec. - در
InteractiveNotificationReceiver
قرارداد،notifyFillOrder
function does not describe any of the parameters. - در
LimitOrderProtocol
قرارداد،DOMAIN_SEPARATOR
function has no NatSpec. - Events and mappings in the
NonceManager
have no NatSpec. - در
OrderRFQMixin
قرارداد،cancelOrderRFQ*
functions do not describe the return values. - در
OrderMixin
contract, several functions lack complete NatSpec. - برخط 168 of
OrderMixin.sol
and on line 71 ofOrderRFQMixin.sol
, it is missing the@dev
برچسب. - Functions in the
PredicateHelper
contract do not describe all of the parameters.
Clear inline documentation is fundamental for outlining the intentions of the code. Mismatches between the inline documentation and the implementation can lead to serious misconceptions about how the system is expected to behave. Consider fixing these errors to avoid confusion for developers, users, and auditors alike.
به روز رسانی: Partially fixed. Misleading documentation addressed in درخواست کشش شماره 75 و درخواست کشش شماره 77.
The 1inch team states:
We’ve fixed misleading docs. Completion of the docs will be done later.
[L11] DoS orders possible when using hooks
La OrderMixin
contract implements functionality to fill generic off-chain swap orders which could have conditions for their success. During order fills, the order can check the predefined “predicate” conditions before continuing with execution.
However, because these predicate conditions could target the logic of any arbitrary contract, a malicious maker could trick takers into believing that an order behaves correctly and that it is valid when checking it off-chain, but then failing when attempting to fill the same order on-chain. This change in the predicate behavior could either be made by frontrunning some variable state on which the predicates depend, by examining the gas sent or even which addresses are involved in the call, or by some other logic.
Furthermore, if the maker defined a interaction during the swapاز interactionTarget
contract could revert itself or revoke the allowance to prevent a successful order fill, essentially leading to the same result as malicious predicates.
Although assets will not be at risk, users or bots finding a favorable order will have the increased burden of trying to identify these sorts of spam orders that can seem legitimate on the surface. In the case that they fail to identify these sorts of orders they will incur wasted gas costs. To reduce the amount of spam orders, consider restricting the available targets for these hooks. Also consider warning users about this possibility before they attempt to fill orders.
به روز رسانی: Not fixed. The 1inch team states:
We handle that on our backend and we’ll think about the ways to notify possible takers about the issue.
[L12] Rounding can be unfavorable for taker
در OrderMixin
و OrderRFQMixin
contracts, when an order is being filled and the taker provides only a makingAmount
or takingAmount
amount, the protocol attempts to calculate the counterpart amount of the swap.
There are two issues with these calculations, the first being that there is no documentation or logic limiting the number of decimals that the amount parameters should use, which we addressed in the Undocumented decimal assumptions موضوع.
The second issue is that, in the course of these calculations, the protocol rounds in the favor of the maker. The rounding issue can be greatly exacerbated when the implicit decimal assumptions are broken, but even when everything is in the expected terms, rounding will occur with small, odd amounts.
Consider allowing the taker to specify a minimum amount of makerAsset
asset that they are willing to receive together with a maximum amount of takerAsset
asset they are willing to swap, so that the acceptance of any rounding is more explicit.
به روز رسانی: Not fixed. The 1inch team states:
Threshold amount should be enough for taker’s protection.
[L13] Contradictory order handling when lacking parameters
در OrderMixin
قرارداد، fillOrderTo
function makes internal calls to the _callGetMakerAmount
و _callGetTakerAmount
functions whenever a fill is attempted and either the makingAmount
یا takingAmount
parameters are zero, respectively, or if the makingAmount
value is larger than the remainingMakerAmount
ارزش.
La _callGetMakerAmount
و _callGetTakerAmount
calls will lead to reversions if the order was not created with the getMakerAmount
or getTakerAmount
parameters, respectively, and a partial fill is being executed.
An inline comment alongside _callGetMakerAmount
و inline comment alongside _callGetTakerAmount
claim that “only whole fills are allowed” if the order was not created with getMakerAmount
or getTakerAmount
پارامترها.
However, there are code paths for which this does not apply, because those paths do not check the length
s of both getMakerAmount
و getTakerAmount
پارامترها.
به طور خاص، وقتی یک taker
مشخص می کند a takerAmount
value for an order which only has a getMakerAmount
, unless that call to getMakerAmount
returns an amount larger than remainingMakerAmount
, a partial fill can be executed in contradiction to the inline documentation.
This leaves the intentionality of those code paths unclear. If this is the expected behavior, consider modifying the inline documentation so that it is more explicit. If this is unintentional behavior, consider always checking the lengths of both the getMakerAmount
و getTakerAmount
parameters simultaneously so that the implementation reinforces the behavior described by the inline documentation.
به روز رسانی: ثابت در درخواست کشش شماره 79.
[L14] Using deprecated Chainlink calls
La ChainlinkCalculator
contract is intended to be used to query Chainlink oracles. It does so via making calls to their latestTimestamp
و latestAnswer
مواد و روش ها، both of which have been deprecated. In fact, the methods are no longer present in the API of Chainlink aggregators as of version three.
To avoid potential future incompatibilities with Chainlink oracles, consider using the latestRoundData
method instead.
به روز رسانی: ثابت در درخواست کشش شماره 67.
یادداشت ها و اطلاعات تکمیلی
[N01] Not importing interfaces
La AggregatorInterface
interface appears to be a subset of code copied from ChainLink
‘s public code repository. The full interface is included in ChainLink
‘s contract npm package.
When possible, to lessen the potential for interface mismatches and resultant issues, rather than re-defining and/or rewriting another project’s interfaces, consider using interfaces installed via their official npm packages instead.
به روز رسانی: ثابت در درخواست کشش شماره 66.
[N02] Deprecated project dependencies
During the installation of the project’s dependencies, NPM warns that one of the packages installed, Highlight
, “will no longer be supported or receive security updates in the future”.
Even though it is unlikely that this package could cause a security risk, consider upgrading the dependency that uses this package to a maintained version.
به روز رسانی: ثابت در درخواست کشش شماره 64.
[N03] External calls to view methods are not staticcalls
Throughout most of the codebase, the protocol explicitly makes external calls via OpenZeppelin’s functionStaticCall
method to restrict the possibility for state changes when they are either not expected or not desirable. However, in the ChainlinkCalculator
contract, despite the intention of making external calls only to view
methods on Chainlink oracles, the external calls in the singlePrice
و doublePrice
functions are not made via explicit staticcall
s.
While we did not identify any immediate security concerns stemming from this, to reduce the attack surface, improve consistency, and clarify intent, consider using explicit staticcall
s, for all external calls to view
توابع در ChainlinkCalculator
قرارداد.
به روز رسانی: Not fixed. The 1inch team states:
We think that syntax complication nullifies improvements in consistency.
[N04] Not failing early with invalid orders
در OrderMixin
قرارداد، fillOrderTo
function handles the special condition when an order has not been previously submitted (remainingMakerAmount == 0
), but it does not explicitly handle the condition when the order is no longer valid (remainingMakerAmount == 1
).
In the latter scenario, the function will eventually revert, but only after burning non-trivial amounts of gas. To clarify intent, increase readability, and reduce gas usage, consider explicitly handling the invalid-order scenario towards the beginning of the function.
به روز رسانی: ثابت در درخواست کشش شماره 68.
[N05] Helper contracts not marked as abstract
In Solidity, the keyword abstract
is used for contracts that are either not functional contracts in their own right, or are not meant to be used as such. Instead, abstract
contracts are inherited by other contracts in the system to create stand-alone functional contracts.
Throughout the codebase, there are examples of helper contracts that are not marked as abstract, despite the fact that they are not meant to be deployed on their own. For instance, the AmountCalculator
, ChainlinkCalculator
, ImmutableOwner
, NonceManager
و PredicateHelper
contracts are all comprised of a base set of functions which are intended to be used by inheriting contracts.
Consider marking helper contracts as abstract
to clearly signify that they are designed solely to add functionality to contracts that inherit them.
به روز رسانی: Not fixed. The 1inch team states:
Those helpers can be deployed separately. They are inherited only for gas savings.
[N06] Inconsistent function ordering
Throughout the codebase, function ordering generally follows the recommended order in the Solidity Style Guide، که: constructor
, fallback
, external
, public
, internal
, private
.
However, in the the OrderMixin
قرارداد، public
checkPredicate
function deviates from the style guide, bisecting the external
توابع.
To improve the project’s overall legibility, consider standardizing function ordering throughout the codebase, as recommended by the Solidity Style Guide.
به روز رسانی: ثابت در درخواست کشش شماره 69.
[N07] Inconsistent order fill flow
La OrderMixin
و RFQOrderMixin
contracts both handle the filling of signed orders, but the general order flow between the two contracts is inconsistent.
OrderMixin
's fillOrderTo
function follows this general flow (pseudo-code):
if ((takingAmount == 0) == (makingAmount == 0))
else if (takingAmount == 0)
else (handle makingAmount == 0) THEN swapTokens
در حالیکه RFQOrderMixin
‘s analogous fillOrderRFQTo
function follows this flow (pseudo-code):
if (takingAmount == 0 && makingAmount == 0)
else if (takingAmount == 0)
else if (makingAmount == 0)
else revert THEN swapTokens
There are no insights from the documentation as to why the first conditional in each of these two functions differs, or why takingAmount
و makingAmount
cannot both be zero in the latter function. Also, the case where both a makingAmount
و یک takingAmount
are provided is much easier to reason about in the fillOrderRFQTo
function, since it is handled clearly in the final else
مسدود کردن.
To clarify intent and increase the overall readability of the code, consider either standardizing the general order flow across these two contracts, or explicitly documenting why the differences exist.
به روز رسانی: Not fixed. The 1inch team states:
This is due to custom pricing functions in limit orders. Since
getMakerAmount
can potentially substantially differ fromgetTakerAmount
, we thought that it is better not to make default option for the taker as it’ll probably confuse them in cases when those getters will be different.
[N08] Error messages are inconsistently formatted or misleading
Throughout the codebase, the require
و revert
error messages, which are meant to notify users of the particular conditions causing a transaction to fail, were found to be inconsistently formatted.
For instance, each one of the error messages on lines 85 از OrderMixin.sol
, 16 از ERC721ProxySafe.sol
و 26 از Permitable.sol
employ a different style.
Additionally, some error messages are misleading:
Error messages are intended to notify users about failing conditions, so they should provide enough information so that appropriate corrections can be made to interact with the system. Uninformative error messages greatly damage the overall user experience, thus lowering the system’s quality. Moreover, inconsistently formatted error messages can introduce unnecessary confusion. Therefore, consider reviewing the entire codebase to make sure every require
و revert
statement has an error message that is consistently formatted, accurate, informative, and user-friendly.
به روز رسانی: تا حدی ثابت شده است درخواست کشش شماره 81.
[N09] Inconsistent use of named return variables
There is an inconsistent use of named return variables in the OrderMixin
contract. Some functions return named variables، دیگران return explicit values، و دیگران declare a named return variable but override it with an explicit return statement.
Consider adopting a consistent approach to return values throughout the codebase by removing all named return variables, explicitly declaring them as local variables, and adding the necessary return statements where appropriate. This would improve both the explicitness and readability of the code, and it may also help reduce regressions during future code refactors.
به روز رسانی: ثابت در درخواست کشش شماره 73.
[N10] Order’s hash calculation is not open to the API
La external
توابع remaining
, remainingRaw
و remainingsRaw
all expect an order hash for successful operation.
However, the helper function _hash
, which returns the hash of an order, has private
visibility. This means that users will have to pack parts of the orders and domain strings manually in order to obtain the hash of an order.
To avoid the potential for mistakes when calculating order hashes and to provide users with a method for generating an order’s respective hash, consider extending the visibility of the _hash
عملکرد به public
and refactoring the name to hash
to be consistent with the rest of the code.
به روز رسانی: ثابت در درخواست کشش شماره 74.
[N11] Semantic overloading of mapping
La _remaining
نقشه برداری در OrderMixin
contract is semantically overloaded to track the status of orders and the remaining amount of assets available for those orders.
The three states that it can take on are:
0
: The order hash has not been seen yet.1
: The order has been either canceled or completely filled.- هر
uint
بزرگتر از1
: The remainingmakerAmount
available to be filled on the order plus1
.
This semantic overloading requires wrapping and unwrapping of this value during lookup
, cancellation
, initialization
و storage
.
Semantic overloading and the necessary logic to enable it can be prone to error and can make the codebase harder to understand and reason about, it may also open the door for regressions in future updates to the code.
To improve the code’s readability, consider tracking the completion state of orders in a separate mapping.
به روز رسانی: Not fixed. The 1inch team cited that a fix would increase gas costs for the fillOrder
تابع.
[N12] Orders with permit allow calls to arbitrary contracts
La OrderMixin
contract inherits the Permitable
contract to allow for single-transaction order filling with assets that accept such permit
calls to modify allowances.
با این حال، تماس به Permitable
قرارداد do not validate whether the target is a permitable asset nor if it is even an asset, which could allow a malicious user to pass the address of an arbitrary contract which could execute another call before the order fill completes.
Although the contract is protected against reentrancy, reducing the attack surface and preventing the calling of external contracts during execution is always recommended. Consider either restricting the asset involved in the permit to the assets involved in the order or to an assets whitelist for the protocol.
به روز رسانی: Not fixed. The 1inch team states:
OrderMixin
does not actually have info about actual tokens asmakerAsset
وtakerAsset
sometimes are proxies or other intermediate contracts and info about actual tokens is stored in some arbitrary bytes. So there is no viable way to restrict which assetpermit
is called on.
[N13] solhint
is never re-enabled
Throughout the codebase, there are a couple of solhint-disable
statements, specifically those on line 23 and on line 41 of RevertReasonParser.sol
, that are not terminated with solhint-enable
.
In favor of explicitness and to be as restrictive as possible when disabling solhint
، استفاده را در نظر بگیرید solhint-disable-line
or solhint-disable-next-line
instead, similar to line 16 of the same file.
به روز رسانی: ثابت در درخواست کشش شماره 72.
[N14] اشتباهات املایی
The codebase contains the following typos:
Additionally the project’s README
(out of scope for this audit) contains the following typos:
برای بهبود خوانایی کد، این اشتباهات تایپی را تصحیح کنید.
به روز رسانی: ثابت در درخواست کشش شماره 71 و درخواست کشش شماره 77.
[N15] Use of uint
بجای uint256
برای حمایت از صراحت، همه موارد از uint
باید به عنوان اعلام شود uint256
. In particular, those in the for
loops on lines 98 و 119 of OrderMixin.sol
و خطوط 16 و 30 of PredicateHelper.sol
.
به روز رسانی: ثابت در درخواست کشش شماره 70.
نتیجه گیری
3 high severity issues were found. Some changes were proposed to follow best practices and reduce the potential attack surface.
- &
- 7
- درباره ما
- دسترسی
- مطابق
- حساب
- در میان
- عمل
- اقدامات
- اضافی
- نشانی
- مزیت - فایده - سود - منفعت
- معرفی
- اجازه دادن
- قبلا
- هر چند
- مقدار
- تحلیل
- API
- روش
- استدلال
- دور و بر
- دارایی
- دارایی
- حسابرسی
- بازگشت پایان
- شروع
- بودن
- بهترین
- بهترین شیوه
- بیت
- رباتها
- ساختن
- صدا
- اهميت دادن
- موارد
- علت
- chainlink
- تغییر دادن
- بررسی
- چک
- رمز
- پیچیده
- شرط
- گیجی
- ساخت و ساز
- شامل
- قرارداد
- قرارداد
- اصلاحات
- هزینه
- میتوانست
- زن و شوهر
- خالق
- واحد پول
- جاری
- داده ها
- مقدار
- خود داری از خدمات
- استقرار
- طرح
- توسعه دهندگان
- پروژه
- DID
- متفاوت است
- مختلف
- دامنه
- دو برابر
- پویا
- در اوایل
- لبه
- تشویق
- به خصوص
- ETH
- واقعه
- حوادث
- همه چیز
- مثال
- تبادل
- انتظار می رود
- تجربه
- بهره برداری
- امکانات
- زمینه
- سرانجام
- نام خانوادگی
- رفع
- انعطاف پذیری
- جریان
- به دنبال
- یافت
- کامل
- تابع
- بودجه
- آینده
- بازی
- GAS
- سوالات عمومی
- دادن
- بزرگ
- راهنمایی
- اداره
- مخلوط
- حس کردن
- داشتن
- کمک
- زیاد
- خیلی
- چگونه
- HTTPS
- ترکیبی
- شناسایی
- تأثیر
- انجام
- مهم
- واردات
- مشمول
- از جمله
- افزایش
- افزایش
- اطلاعات
- اطلاعات
- شالوده
- بینش
- قصد
- علاقه
- رابط
- گرفتار
- مسائل
- IT
- بزرگ
- بزرگتر
- رهبری
- برجسته
- کتابخانه
- محدود شده
- لاین
- نقدینگی
- ذکر شده
- لیست
- محلی
- نگاه
- مراجعه
- عمده
- سازنده
- ساخت
- بازار
- mempool
- آینه
- مدل
- اکثر
- محبوبترین
- از جمله
- ویژگی های جدید
- غیر قابل مبادله
- نشانه های غیر قارچی
- رسمی
- باز کن
- عملیات
- گزینه
- وحی
- سفارش
- سفارشات
- دیگر
- مالک
- الگو
- محبوب
- در حال حاضر
- جلوگیری
- قیمت
- قیمت گذاری
- خصوصی
- روند
- پروژه
- پروژه ها
- حفاظت
- پروتکل
- ارائه
- فراهم می کند
- پروکسی
- عمومی
- منتشر کردن
- خرید
- کیفیت
- بالا بردن
- واقعیت
- كاهش دادن
- اعتماد
- گزارش
- گزارش ها
- مخزن
- مورد نیاز
- REST
- نتایج
- بازده
- این فایل نقد می نویسید:
- خطر
- دور
- دویدن
- sdk
- تیم امنیت لاتاری
- خدمات
- تنظیم
- به اشتراک گذاشته شده
- سهام
- تغییر
- مشابه
- ساده
- کوچک
- هوشمند
- قراردادهای هوشمند
- So
- استحکام
- اسپم
- به طور خاص
- هزینه
- Spot
- شروع
- دولت
- بیانیه
- ایالات
- وضعیت
- ذخیره سازی
- سبک
- ارسال
- موفقیت
- موفق
- موفقیت
- پشتیبانی
- پشتیبانی
- سطح
- گزینه
- سیستم
- هدف
- آزمون
- تست
- تست
- از طریق
- سراسر
- زمان
- با هم
- رمز
- نشانه
- مسیر
- پیگردی
- معامله
- اعتماد
- منحصر به فرد
- به روز رسانی
- us
- قابلیت استفاده
- دلار آمریکا
- USDT
- کاربران
- سودمندی
- ارزش
- چشم انداز
- دید
- صبر کنيد
- چی
- لیست سفید
- در داخل
- بدون
- مهاجرت کاری
- با ارزش
- صفر