Table of Contents
${issue-summary-table}
We audited the AmperaFoundation/sol-contracts repository at commit 4c44237 of the staging branch.
In scope were the following contracts:
contracts
├── Collateral.sol
├── LetterOfCredit.sol
├── Pricing.sol
├── PythPriceOracle.sol
└── interfaces
├── ICollateral.sol
├── ICollateralDepositTarget.sol
├── ILiquidatable.sol
├── ILiquidator.sol
└── IPriceOracle.sol
Anvil is an Ethereum-based collateral management protocol. It defines the Collateral contract as a central point for transparent and efficient collateral management, which exists within a system of approved collateralizable contracts. For instance, the LetterOfCredit contract is a collateralizable contract serving a special purpose.
The Collateral contract behaves like a collateral vault within the system. A user is able to open an account in it by depositing some amount of any allowed collateral token. Then, the user can interact with one or more collateralizable contracts, and instead of performing token transfers for each one of them, the collateralizable contract interacts with the Collateral contract on behalf of the user. For example, if a collateralizable contract needs to ensure that a specific collateral amount is dedicated to its users' actions, it requests a collateral reservation within the Collateral contract.
A collateralizable contract can also claim some actual reserved collateral amount. In this case, an actual transfer between the Collateral contract and the collateralizable contract takes place. In addition, it is possible for the collateralizable contract to modify a collateral reservation, either by increasing or decreasing the reserved collateral of a user's account. It is also possible to completely release the reservation.
Note that all the above operations require the collateralizable contract to be whitelisted by the administrator (owner) of the Collateral contract. Moreover, it must also be approved by the user within the Collateral contract. Furthermore, a protocol fee is applied whenever a user withdraws collateral or the collateralizable contract claims some amount from Collateral.
The LetterOfCredit contract allows users to open two kinds of Letters of Credit (LoCs) provided that certain requirements are met. By opening an LoC, the user specifies a collateral amount that can be redeemed as credit by a specified beneficiary until a specified expiration date.
Two types of LoCs are associated with the credited token: if the credited token is the same as the collateral token then the LoC is converted, otherwise it is unconverted. A converted LoC is rather simple: the beneficiary can choose to redeem part or all of the credited amount at any point until the expiration timestamp or completely cancel the LoC, releasing the LoC creator's collateral.
In an unconverted LoC (uLoC) the credited token is different from the collateral token. The uLoC creator specifies the collateral amount to be reserved as well as the credited amount, which is considered the LoC's face value. It is important that the beneficiary is able to redeem the whole LoC's face value at any point until the LoC's expiration, given the corresponding collateral/credit token price. For this reason, the protocol defines a collateral factor (CF) per allowed token pair, which should be respected or else the LoC is considered unhealthy and is subject to liquidation.
Under healthy LoC conditions, both the creator and the beneficiary encounter some financial risk depending on the price of the collateral/credited tokens. If the collateral's price is decreasing then the creator's risk increases and vice versa. While the beneficiary is able to redeem at any time, the creator is also able to modify the LoC's collateral amount, provided that the collateral factor is respected given the current price. In addition, the creator is able to convert the LoC at any time, essentially letting the beneficiary redeem the LoC's face value in the credited token, instead of the collateral token.
Liquidators are incentivized to act on unhealthy LoCs through a liquidation fee, which lets them buy an LoC's collateral at a discount. During liquidations, the liquidator pays for the collateral in the credited token and the LoC is considered converted toward the LoC's credited token and amount.
The shared logic between liquidations and redemptions makes some parts of the system unintuitive. For instance, a valid redemption by the beneficiary results in the distribution of the liquidation bonus to the beneficiary on top of the redeemed amount. This results in an additional amount being taken from the creator's collateral once redeemed.
The system uses the Pyth Network price feeds to retrieve reliable prices which is of vital importance to the protocol. The PythPriceOracle contract performs the task of fetching and validating the prices from the oracle.
The contracts in the system are not upgradeable in the typical sense (i.e., using proxy contracts). Instead, the owner has the ability to specify an upgrade target in the Collateral contract, which must implement the ICollateral interface. Users are then able to trigger their account's migration and send their available balance to this contract without being charged the usual withdrawal fee. Note that reserved collateral is not withdrawable until the associated LoC positions are closed.
The Collateral, LetterOfCredit and PythPriceOracle contracts are ownable. The owner has access to sensitive operations and is thus trusted to always act to the benefit of the protocol's users. More specifically, the owner has the following privileges:
In the PythPriceOracle contract:
uLoC.In the Collateral contract:
withdrawalFeeBasisPointsICollateral upgrade targetIn the LetterOfCredit contract:
The PythPriceOracle.updatePrice function returns a price after decoding the input and output token addresses from the provided bytes argument. This price is used in several locations in the LetterOfCredit contract whenever an _oraclePriceUpdate argument is provided with non-zero length, such as when an LoC is created, redeemed, or liquidated.
The provided _oraclePriceUpdate contains token arguments which are never validated in the LetterOfCredit contract. This validation is necessary to ensure they match the corresponding collateral and credited tokens of the current operation. As a result, any tokens with valid price feeds configured in the oracle contract can be used.
For example, the collateral and credited token addresses can be swapped to force the inverse price to be used. This causes a number of issues, but some extreme examples are:
Consider decoding and validating the provided price update bytes to ensure that they match the collateral and credited token addresses, respectively.
Update: Resolved in pull request #134.
Token prices in terms of USD are retrieved from the Pyth oracle. It returns the PythStructs.Price struct that includes an integer value price along with an exponent value to scale the price by.
In order to calculate an output amount of tokens per unit input (input price), the PythPriceOracle contract performs the following calculation using the Pyth values:
outputPerUnitInput = inputPrice * 10**(inputDecimals - outputExponent + inputExponent + 1) / outputPrice
It then sets the exponent of the output to be (-outputDecimals - 1).
However, this calculation is incorrect. It affects the scale of the input price to such a degree that any operation in the LetterOfCredit contract involving prices is essentially rendered unusable (e.g., creating, redeeming, or liquidating uLoCs). The calculation should instead be,
outputPerUnitInput = inputPrice * 10**outputDecimals / outputPrice
with an exponent value of inputExponent - outputExponent - inputDecimals.
Consider correcting the calculation to return the price with the correct scale. Moreover, consider simplifying it to avoid trying to have a fixed output exponent of (-outputDecimals - 1) which makes things more error-prone. For correct calculation, it should be sufficient to use a precision buffer to prevent precision loss during the division.
Update: Resolved in pull request #135.
The liquidation reward is calculated slightly differently for insolvent and solvent LoCs, resulting in a slightly lower fee percentage in the insolvent case. This small inconsistency is considered acceptable due to the special circumstances of an insolvent LoC.
However, the liquidation fee for an insolvent LoC is essentially a portion of the claimable collateral amount. On the other hand, for a solvent LoC, the fee is an extra amount added on top of the collateral to be typically claimed. Therefore, it is possible that when liquidating a solvent LoC, the liquidation bonus plus the claimable collateral brings the LoC into an insolvent state. However, it is still treated as solvent. This causes the transaction to unexpectedly revert at a later point because the position will not have enough collateral to cover the full amount. In this case, the LoC cannot be liquidated until the token prices change sufficiently in either direction.
Consider performing some extra checks when the liquidation fee is calculated such that the final amount to be claimed is well-covered by the reserved collateral. Otherwise, treat the LoC as insolvent.
Update: Resolved in pull request #150.
_redeemThe last code block of the _redeem function in the LetterOfCredit contract updates the LoC's state in storage upon its redemption. However, when an uLoC is redeemed, the storage is updated before the execution reaches that point. This is because, for the uLoCs, theliquidateLOCCollateral function is called to convert the necessary amounts. As part of this conversion step, the LoC's parameters are updated in storage either when a partial or a full redemption takes place.
The current implementation does not appear to pose an immediate security risk as the storage is overwritten with the correct values. However, the code is not clear and it is non-trivial to understand its security and consistency. This could possibly result in bugs introduced in a later version of the contract. In addition, there is increased gas consumption due to the duplicate update operations.
Consider refactoring this part of the _redeem function to appropriately update the state only for converted LoCs. This would help reduce gas consumption and increase the overall clarity of the codebase.
Update: Resolved in pull request #132.
The prices fetched by the Pyth network come with a degree of uncertainty which is expressed as a confidence interval around the given price values. Considering a provided price p, its confidence interval σ is roughly the standard deviation of the price's probability distribution. The official documentation of the Pyth Price Feeds recommends some ways in which this confidence interval can be utilized for enhanced security. For example, the protocol can compute the value σ / p to decide the level of the price's uncertainty and disallow user interaction with the system in case this value exceeds some threshold.
Currently, the protocol completely ignores the confidence interval provided by the price feed. Consider utilizing the confidence interval provided by the Pyth price feed as recommended in the official documentation. This would help mitigate the possibility of users taking advantage of invalid prices.
Update: Resolved in pull request #148. The Ampera team stated:
The approach taken is to "sanity check" the price making sure that the Confidence Interval is smaller than the price, as we are largely relying on the oracle to have a good price. If somehow there is an issue with the oracle, related to Confidence Intervals or not, we will use the implemented logic to upgrade the
IPriceOraclewithinLetterOfCredit.
In the Collateral contract, the calculation of the amountWithFee amount within the claimCollateral function is not consistent with the formula of the getClaimableAmount function. More specifically, the formula in the claimCollateral function is currently:
amountWithFee = (_amountToReceive * (10_000 + feeBasisPoint)) / 10_000
Whereas according to getClaimableAmount it should be:
amountWithFee = (_amountToReceive * 10_000) / (10_000 - feeBasisPoint)
The latter formula results in a slightly greater amountWithFee and is also used in reserveClaimableCollateral. This results in slightly more collateral being reserved to account for the fee, compared to the calculation in claimCollateral, which will calculate less fee. Hence, it is possible to claim slightly more collateral due to the fee discrepancy.
Consider using the same formula consistently when calculating the claimable or withdrawal fee amount. This will help avoid accounting errors in the contract.
Update: Resolved in pull request #151.
When a beneficiary or authorized address redeems a uLoC, they need to send the credited amount to the LetterOfCredit contract as a consequence of sharing liquidation logic, for example, here. They are eventually sent back the credited amount during the call.
This is cumbersome, and unnecessary to perform the redemption. The beneficiary/authorized user needs to have the credited token amount balance in their wallet, and also approve the LetterOfCredit to spend the credited amount, thereby wasting gas. The beneficiary may not have the means to obtain the balance required for this transfer and may be unable to redeem the tokens they are owed.
Consider removing the credited token transfers during redeem calls to avoid this unnecessary action.
Update: Resolved in pull request #152.
In order to conform to the EIP-165 specification, the Collateral and PythPriceOracle contracts define the supportsInterface function which checks whether each contract satisfies a specific interfaceId.
The IDs of each contract's supported interfaces are computed and stored in constant variables for the needs of the supportsInterface function. However, the recent versions of the Solidity compiler provide a dedicated command to automatically compute an interface's ID.
Consider simplifying the code by using the interfaceId command to automatically compute the IDs of the supported interfaces.
Update: Resolved in pull request #129.
There are several cases of code duplication which make the codebase harder to read. In addition, they also make it more error-prone, harder to fix, and complicate maintainability and upgradeability. Most notably:
_liquidateLOCCollateral function of the LetterOfCredit contract:
liquidatorAddress changes. Consider moving these actions at the end of the function.claimableCollateralUsed could be defined once right after the call to _calculateLiquidationContext._collateralUsed can be assigned once at the end of the function and before calling _markLOCPartiallyLiquidated, _markLOCConverted._senderOrAuthorizer argument is different.refundExcess is separately but equivalently implemented in the LetterOfCredit and PythPriceOracle contracts. Consider creating an abstract contract for the definition of the refundExcess modifier which will be inherited by the contracts that need to use it.Consider refactoring the code for the above cases. This will lower gas consumption and increase the codebase's overall readability and quality.
Update: Resolved in pull request #136.
The claimCollateral function of the Collateral contract can be called from the collateralizable contract to claim a portion (or all) of a reservation's collateral. In case only part of the reserved collateral is claimed, it is possible to either completely close the collateral reservation by adding any remainder to the account's available collateral or keep the reservation open.
In the latter case, it is possible that only a very small reserved amount remains so that the corresponding claimable amount is zero. A similar case of reserving too little collateral resulting in zero claimable amount is prohibited within the _reserveCollateral function.
The same result is possible when a user calls modifyCollateralReservation to decrease the reserved amount.
Consider reverting partial claims or collateral modifications which result in zero claimable collateral. This will help ensure consistency and avoid active reservations that cannot be practically used.
Update: Resolved in pull request #137.
The owner of the Collateral contract can approve addresses using code supporting the ICollateral interface as an account upgrade target. Accounts are then able to call upgradeAccount to send all or a portion of their available collateral to the new contract.
In doing so, the account balances are updated in the current contract, and the new contract is approved to spend the amounts before the depositToAccount call. However, upgradeAccount only approves the new contract to transfer the tokens, and does not verify if the tokens are actually sent. If the new contract is unable to complete the transfer, the tokens will be unrecoverable in the old contract, except by the owner.
Consider verifying that token balances are actually updated when users upgrade their accounts.
Update: Partially resolved in pull request #153. More specifically, instead of checking the balance when an account is upgraded, specific guidelines on how to implement the deposit function of ICollateralDepositTarget are documented. The Ampera team stated:
We decided to add documentation to the function to be called to add approved upgrade contracts, as well as to the deposit target interface because:
- We'd do testing prior to adding a supported target upgrade contract to ensure this isn't an issue.
- "Lost" funds can be captured via governance if we somehow get into this situation.
- Spending gas on each upgrade to check that each token was transferred seems unnecessary, given 1 & 2.
There are some cases where state variables are defined as integers of less than 256 bits despite the fact that they cannot be packed in a single storage slot. Some identified instances of this are:
collateralReservationNonce variable in the Collateral contract which is placed between two mappingslocNonce variable in the LetterOfCredit contractmaxPriceUpdateSecondsAgo and maxLocDurationSeconds variables in the LetterOfCredit contractConsider using uint256 integer variables whenever it is not possible to pack contiguous variables into a single storage slot. This will lead to gas savings upon reading or writing them.
Update: Resolved in pull request #138.
We identified some docstrings that are either incomplete or misleading. More specifically:
In the Collateral.sol contract:
withdraw function documents that withdrawing a specified amount will entail lowering a user's balance by more than that amount to account for the fee. However, the fee is actually taken as a portion of the amount, so the user's balance is just lowered by that amount and they receive a slightly less amount.In the LetterOfCredit.sol contract:
upsertCreditedTokensAsOwner function, as well as that of the corresponding private function, states that "setting max per LoC or global max to 0 effectively disables a CreditedToken". However, it is impossible to set maxPerLOC equal to 0 due to the requirement minPerLOC < maxPerLOC. Consider updating these docstrings for accuracy.Consider updating the docstrings as described above to enhance the readability and overall quality of the codebase.
Update: Resolved in pull request #139.
Several cases were identified throughout the codebase where the code could be updated to be clearer and more readable:
Collateral contract, the functions claimCollateral, releaseAllCollateral and modifyCollateralReservation retrieve information from the collateral reservation of interest. While functions claimCollateral and releaseAllCollateral name the corresponding internal variable as collateral, the modifyCollateralReservation function names it reservation. Consider adopting consistent naming across functions.CollateralToken struct in the Collateral contract contains an entry for the token's decimals which is not used anywhere within the contract. Note that this information can also be fetched by the token's contract itself. Nevertheless, it is declared as uint8 despite the fact that it cannot be packed with the following entries of the struct and thus would consume more gas to be read/written. Consider removing the decimals entry from the CollateralToken struct as it is redundant. If it is needed for UI-related reasons, consider declaring it as a uint256 for clarity.Update: Partially resolved in pull request #140. The Ampera team stated:
We decided not to update the mapping field names because:
- They are already referenced at many places throughout the various codebases, and it would be tricky to do so without introducing issues.
- In all cases they follow a convention:
- We don't use arrays, so all plural variable names are mappings.
- Mappings of structs are keyed by their logical ID (address for ERC-20s and token-related structs, id field for other structs).
- All mappings of permissions are
address=>bool(e.g.,collateralizableContracts,permittedCollateralUpgradeContracts).- Variable names in which there is a mapping of
address=>address=> something clearly stating what the first address should be and then fall back to mapping rules in 1, 2, & 3 (e.g.,accountBalancesoraccountCollateralizableApprovals).
minPerLoc Is Not Checked Against Zero Upon LoC CreationConsider requiring that minPerLoc is set to a non-zero value in the _upsertCreditedTokensAsOwner function of the LetterOfCredit contract. This would revert the creation of a zero-credit LoC at an early point and improve code clarity.
Note that although this constraint is implicitly imposed by the non-zero collateral factor requirement, the overall security and quality of the codebase would benefit from checking the protocol's constraints at all possible places. Such constraints make it easier to understand possible edge cases and security considerations in other parts of the protocol (e.g., when a LoC's credited amount is validated).
Update: Resolved in pull request #141.
uint Instead of uint256There are some instances of using uint instead of uint256:
Pricing.solPythPriceOracle.solIn favor of explicitness and consistency, consider replacing all instances of uint with uint256.
Update: Resolved in pull request #142.
The Collateral and PythPriceOracle contracts both import the IERC165 interface. This is redundant since both contracts already inherit ERC165.
Consider removing the unnecessary import to improve the overall clarity and readability of the code.
Update: Resolved in pull request #129.
In the Collateral contract, the _collateralUpgradeContractAddress variable in the upsertCollateralUpgradeContractApproval function is unnecessarily casted.
To improve the overall clarity, intent, and readability of the code, consider removing this unnecessary cast.
Update: Resolved in pull request #144.
console Imports in CommentsThere are Hardhat console imports in the comments:
Collateral.solLetterOfCredit.solPythPriceOracle.solConsider removing these comments to improve the clarity of the codebase.
Update: Resolved in pull request #145.
@inheritdoc TagExternal functions in the PythPriceOracle.sol contract copy exact docstrings from the IPriceOracle.sol interface.
Consider using the @inheritdoc tag wherever possible in the PythPriceOracle.sol contract to make the code more concise.
Update: Resolved in pull request #146.
The ILiquidator interface defines the liquidate function as payable. However, this function does not receive ETH.
Consider removing the payable keyword from the function signature in the interface. This will help improve clarity and better communicate the intention to implementers.
Update: Resolved in pull request #147.
Anvil is an Ethereum-based collateral management protocol which utilizes a central Collateral contract to efficiently manage and allocate collateral for various approved collateralizable contracts. It aims to streamline the interaction between users and these contracts by allowing the Collateral contract to handle collateral reservations on behalf of users.
Throughout the audit, the Ampera team has been very responsive, providing valuable insights and detailed explanations while engaging in fruitful discussions with the auditors. The documentation provided by them helped the auditors to quickly comprehend the protocol.
It is highly recommended to write additional test cases in order to achieve 100% test coverage, especially for the LetterOfCredit and PythPriceOracle contracts. For the latter, testing with real data from the Pyth API and the on-chain contract (in a forked environment or testnet) is advised. This would help assess the complete price calculation behavior and verify that it is accurate.