We audited the Ion-Protocol/ion-seaport repository at commit 54110bb. In scope were the following files:
src
├── SeaportBase.sol
├── SeaportDeleverage.sol
├── SeaportLeverage.sol
└── interfaces
├── IGemJoin.sol
├── IIonPool.sol
├── ISpotOracle.sol
├── IUFDMHandler.sol
└── IWhitelist.sol
Additionally, we audited the Ion-Protocol/ion-protocol repository at commit 0a8c0d2. For files below labelled with "diff", only changes since the 1c42b94 commit were considered.
src
├── Constants.sol (diff)
├── flash
│ ├── UniswapFlashswapDirectMintHandlerWithDust.sol
│ └── lrt
│ ├── EzEthHandler.sol
│ └── RsEthHandler.sol
├── interfaces
│ └── ProviderInterfaces.sol (diff)
├── libraries
│ └── lrt
│ ├── KelpDaoLibrary.sol
│ └── RenzoLibrary.sol
└── oracles
├── reserve
│ └── lrt
│ ├── EzEthWstEthReserveOracle.sol
│ └── RsEthWstEthReserveOracle.sol
└── spot
└── lrt
├── EzEthWstEthSpotOracle.sol
└── RsEthWstEthSpotOracle.sol
The audited code within the "ion-seaport" repository centers around two contracts, the SeaportLeverage and SeaportDeleverage contracts. In the leverage case, a user is able to obtain collateral, deposit it into Ion and borrow against it, using the proceeds of the borrow to pay back the Seaport order creator. The deleverage case works in reverse, allowing a user to obtain the borrow asset, repay their borrow in Ion, and withdraw their collateral to repay the Seaport order creator. These contracts simply fulfill orders on Seaport, which must be created outside of the audited codebase prior to fulfilling them.
Both contracts utilize function selector clashing to hijack the transferFrom call that Seaport makes during order fulfillment. Within the "leverage" case, this is used to deposit tokens into Ion pools and borrow assets, which are then used to fulfill the remainder of the order. In the "deleverage" case, this is performed in reverse.
It should be noted that the Seaport protocol is complex, and thus various precautions are taken by Ion to constrain the order parameters and prevent unexpected behavior. The "leverage" functionality is limited to only whitelisted users, and both the "leverage" and "deleverage" functions can only be used to change the balances of the caller's own account.
The audited code within the "ion-protocol" repository implements the necessary contracts to allow Renzo "ezETH" and KelpDAO "rsETH" as collateral assets in the Ion system. There are new contracts which are the "handlers" and the "libraries" for these collaterals. For ezETH, there is a new version of the UniswapFlashswapDirectMintHandler contract called UniswapFlashswapDirectMintHandlerWithDust, which exists to help mitigate the problem of "ezETH" not being mintable in exact amounts. Due to the mathematics within ezETH, minting can only occur at certain values, and it is very likely that a user will not be able to receive exactly a specified amount out. Ion has also asked OpenZeppelin to investigate the error incurred in ezETH minting calculations, and some informal recommendations have been provided outside of this audit report. Contracts which price ezETH and rsETH against wstETH, the intended the borrow asset, have also been added. These contracts determine liquidation thresholds and maximal borrow amounts. They rely on internal state of the ezETH and rsETH contracts, as well as Chainlink and Redstone oracles, to compute fair values for both collaterals.
Ion Protocol integrates with many other protocols deployed on Ethereum. As such, it is generally assumed that those protocols behave as documented. While there are many integrations associated with Ion, the integrations relevant to this specific engagement are:
As noted above, it is assumed that integrated contracts behave as described in their respective documentation. More specifically, we make the following assumptions:
Constants.sol are accurate and will match the values used in the deployed Ion contracts.We encourage the Ion team to consider implementing monitoring for their deployed contracts.
Constants.sol. If an upgrade is detected, scan all execution flows which Ion depends on for any changes in the Renzo contract logic. Additionally, identify all changes to logic pertaining to values like TVL or ezETH total supply, which Ion also depends on.rsEthPrice (from the RSETH_LRT_ORACLE) which Ion also depends on.spot address for a given collateral, in the event of an upgrade to any collateral contracts.EzEthHandler should be updated.Additionally, before performing certain actions, we encourage Ion to validate the following:
The Ion team specifically requested a review of the math used for ezETH integration within the Ion protocol. The audit team has relayed their findings regarding this asset to Ion outside of this audit report. In summary, the findings were that the minting of ezETH cannot necessarily result in any arbitrary amount specified.
Generally, minting behavior of ezETH is determined by the intermediate value, inflationPercentage, which is used for computing the TVL growth of a deposit into ezETH and the subsequent growth of the supply of ezETH. Since inflationPercentage has a maximal value of 1e18 - 1, there are thus that many different values for the amount of ezETH which can be minted. There are possibly many more potential values for the amount of ETH which can be deposited, and thus the inflationPercentage is the limiting factor in potential values of ezETH minted.
We encourage the Ion team to assess the tradeoff between increased gas costs and potential savings to users when deciding to make computations more accurate. Consider the typical values of deposits into the Ion protocol and utilize these to compute potential error as a general range of values. In addition, extensively document the error considerations to reassure users who may be concerned about inaccuracy in the computations.
In the seaportCallback4878572495 function within the SeaportDeleverage contract, there is a requirement that the BASE token balance of the contract is 0 after a full deleverage.
However, this will always revert if a user has independently transferred any amount of BASE tokens into the SeaportDeleverage contract. Since there is no way to transfer the tokens out of the contract, a single transfer will permanently brick the contract.
Consider removing this check, or modifying it to instead check that the token balance of the contract has not changed since the beginning of the execution flow. Alternatively, consider either modifying the logic to transfer all unneeded BASE tokens to the user, or implementing a "rescue" function to remove stranded BASE tokens.
Update: Resolved in pull request #3. The sanity check was removed to prevent bricking of the contract.
Throughout the codebase there are several parts that have incomplete docstrings. For instance:
getPrice function in EzEthWstEthSpotOracle.sol the return value is partially documentedflashswapAndMint function in UniswapFlashswapDirectMintHandlerWithDust.sol the deadline parameter is not documented.Consider thoroughly documenting all functions/events (and their parameters or return values) that are part of any contract's public API. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved in pull request #76.
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. For instance, the files IGemJoin.sol, IIonPool.sol, IUFDMHandler.sol, IWhitelist.sol have the solidity ^0.8.4 floating pragma directive.
Consider using a fixed pragma version.
Update: Acknowledged, not resolved. The Ion Protocol team stated:
The floating pragma was kept in order to allow other projects using a different Solidity version to use this repo as a submodule.
ILK_INDEX in Seaport Leveraging ContractsThe SeaportDeleverage contract uses an immutable ILK_INDEX which is set in the constructor of the inherited SeaportBase contract.
There is an inconsistency in that SeaportDeleverage assumes an index of 0 in many places in both the deleverage and seaportCallback4878572495 functions, while SeaportLeverage.sol uses ILK_INDEX. This inclusion of the ILK_INDEX variable suggests that the contract should work with any ilk. However, the SeaportDeleverage contract would not function if the selected ilk index was not 0.
Consider using a consistent Ilk Index across the "Leverage" and "Deleverage" contracts.
Update: Resolved in pull request #4. The SeaportDeleverage contract was modified to use a hardcoded ILK_INDEX of 0.
Since Solidity version 0.8.0, any arithmetic operation automatically checks for over- and underflows, which cost gas. Since it is highly unlikely that a positively incrementing variable will overflow within a loop, the increment could be wrapped into an unchecked block.
This applies to the following instances:
RenzoLibrary.solRenzoLibrary.solTo improve gas consumption, consider wrapping the incremental update into an unchecked block to save the gas required to check against overflows.
Update: Resolved in pull request #74. The incremented update was removed in an unrelated refactoring.
Throughout the codebase, there are unused errors. For instance:
ZoneHashMustBeZero error in SeaportBase.sol.InvalidAmountIn error in RenzoLibrary.sol.To improve the overall clarity, intentionality, and readability of the codebase, consider either using or removing any currently unused errors.
Update: Resolved in pull request #74 and pull request #5.
In SeaportDeleverage.sol, the import of IWhitelist.sol is unused and can be removed.
Consider removing unused imports to improve the overall clarity and readability of the codebase. Alternatively, if it is intended to use the whitelist within SeaportDeleverage.sol, consider implementing it.
Update: Resolved in pull request #5.
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 lack of knowledge on how to do so. Additionally, if the contract incorporates third-party libraries and a bug surfaces in these, it becomes easier for the maintainers of those libraries to make contact with the appropriate person about the problem and provide mitigation instructions.
Consider adding a NatSpec comment containing a security contact on top of the contracts definition. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.
Update: Resolved in pull request #5 for the ion-seaport repository and in pull request #76 for the ion-protocol repository.
transfer/transferFrom CallSome early implementations of ERC-20 tokens would return false instead of reverting the transaction when a transfer failed. This behavior is considered unsafe because a false return value could be ignored by the caller, leading to unexpected behavior.
In SeaportDeleverage.sol, the transfer call is unchecked.
Consider to use SafeERC20 OpenZeppelin library to perform safe token transfers. Note that with the current application, only ETH staking derivatives that revert on transfer are being called with transfer. However, in the case that the code is modified to support other "base" assets in the future, this will protect against unexpected behavior during transfer calls.
Update: Resolved in pull request #5.
The following typographical errors have been identified:
UniswapFlashswapDirectMintHandlerWithDust, "leverge" should be "leverage".SeaportDeleverage, "to to" should be "to"Consider correcting the typos for better readability and a more professional codebase.
Update: Resolved in pull request #76 and pull request #5.
One high-severity vulnerability was found in the contracts which were quickly acknowledged by the Ion team. Both the ion-protocol and ion-seaport codebases were covered by unit and fuzz tests. The Renzo ETH contracts were designed to deposit the "optimal" amount such that no excess ETH is deposited for any amount of ezETH. We evaluated that Ion implemented this correctly. However, this optimised amount makes a very small difference compared to imperfect deposit amounts. The Ion team was very helpful in documenting and explaining the codebase and clarifying the system's design.
OpenZeppelin classifies smart contract vulnerabilities on a 5-level scale:
This classification is applied when the issue’s impact is catastrophic, threatening extensive damage to the client's reputation and/or causing severe financial loss to the client or users. The likelihood of exploitation can be high, warranting a swift response. Critical issues typically involve significant risks such as the permanent loss or locking of a large volume of users' sensitive assets or the failure of core system functionalities without viable mitigations. These issues demand immediate attention due to their potential to compromise system integrity or user trust significantly.
These issues are characterized by the potential to substantially impact the client’s reputation and/or result in considerable financial losses. The likelihood of exploitation is significant, warranting a swift response. Such issues might include temporary loss or locking of a significant number of users' sensitive assets or disruptions to critical system functionalities, albeit with potential, yet limited, mitigations available. The emphasis is on the significant but not always catastrophic effects on system operation or asset security, necessitating prompt and effective remediation.
Issues classified as being of a medium severity can lead to a noticeable negative impact on the client's reputation and/or moderate financial losses. Such issues, if left unattended, have a moderate likelihood of being exploited or may cause unwanted side effects in the system. These issues are typically confined to a smaller subset of users' sensitive assets or might involve deviations from the specified system design that, while not directly financial in nature, compromise system integrity or user experience. The focus here is on issues that pose a real but contained risk, warranting timely attention to prevent escalation.
Low-severity issues are those that have a low impact on the client's operations and/or reputation. These issues may represent minor risks or inefficiencies to the client's specific business model. They are identified as areas for improvement that, while not urgent, could enhance the security and quality of the codebase if addressed.
This category is reserved for issues that, despite having a minimal impact, are still important to resolve. Addressing these issues contributes to the overall security posture and code quality improvement but does not require immediate action. It reflects a commitment to maintaining high standards and continuous improvement, even in areas that do not pose immediate risks.