The audit makes no statements or warranties about utility of the code, safety of the code, suitability of the business model, investment advice, endorsement of the platform or its products, regulatory regime for the business model, or any other statements about fitness of the contracts to purpose, or their bug free status. The audit documentation is for discussion purposes only. The information presented in this report is confidential and privileged. If you are reading this report, you agree to keep it confidential, not to copy, disclose or disseminate without the agreement of the Client. If you are not the intended recipient(s) of this document, please note that any disclosure, copying or dissemination of its content is strictly forbidden.
A group of auditors are involved in the work on the audit. The security engineers check the provided source code independently of each other in accordance with the methodology described below:
- Project documentation review.
- General code review.
- Reverse research and study of the project architecture on the source code alone.
- Build an independent view of the project's architecture.
- Identifying logical flaws.
- Manual code check for vulnerabilities listed on the Contractor's internal checklist. The Contractor's checklist is constantly updated based on the analysis of hacks, research, and audit of the clients' codes.
- Code check with the use of static analyzers (i.e Slither, Mythril, etc).
Eliminate typical vulnerabilities (e.g. reentrancy, gas limit, flash loan attacks etc.).
- Detailed study of the project documentation.
- Examination of contracts tests.
- Examination of comments in code.
- Comparison of the desired model obtained during the study with the reversed view obtained during the blind audit.
- Exploits PoC development with the use of such programs as Brownie and Hardhat.
Detect inconsistencies with the desired model.
- Cross check: each auditor reviews the reports of the others.
- Discussion of the issues found by the auditors.
- Issuance of an interim audit report.
- Double-check all the found issues to make sure they are relevant and the determined threat level is correct.
- Provide the Client with an interim report.
- The Client either fixes the issues or provides comments on the issues found by the auditors. Feedback from the Customer must be received on every issue/bug so that the Contractor can assign them a status (either "fixed" or "acknowledged").
- Upon completion of the bug fixing, the auditors double-check each fix and assign it a specific status, providing a proof link to the fix.
- A re-audited report is issued.
- Verify the fixed code version with all the recommendations and its statuses.
- Provide the Client with a re-audited report.
- The Customer deploys the re-audited source code on the mainnet.
- The Contractor verifies the deployed code with the re-audited version and checks them for compliance.
- If the versions of the code match, the Contractor issues a public audit report.
- Conduct the final check of the code deployed on the mainnet.
- Provide the Customer with a public audit report.
All vulnerabilities discovered during the audit are classified based on their potential severity and have the following classification:
| Severity | Description |
|---|---|
| Critical | Bugs leading to assets theft, fund access locking, or any other loss of funds. |
| High | Bugs that can trigger a contract failure. Further recovery is possible only by manual modification of the contract state or replacement. |
| Medium | Bugs that can break the intended contract logic or expose it to DoS attacks, but do not cause direct loss funds. |
| Low | Bugs that do not have a significant immediate impact and could be easily fixed. |
Based on the feedback received from the Customer regarding the list of findings discovered by the Contractor, they are assigned the following statuses:
| Status | Description |
|---|---|
| Fixed | Recommended fixes have been made to the project code and no longer affect its security. |
| Acknowledged | The Customer is aware of the finding. Recommendations for the finding are planned to be resolved in the future. |
Curve Lending allows users to borrow crvUSD against any collateral token or to borrow any token against crvUSD, while benefiting from the soft-liquidation mechanism provided by LLAMMA. This innovative approach to overcollateralized loans enhances risk management and user experience for borrowers. Additionally, Curve Lending allows users to generate interest through lending (supplying) their assets to be borrowed by others.
| Title | Description |
|---|---|
| Client | Curve Finance |
| Project name | Curve Lending |
| Timeline | 06 February 2024 - 31 May 2024 |
| Number of Auditors | 3 |
| Date | Commit Hash | Note |
|---|---|---|
| 30.05.2023 | c5169a7eb687a9878b989696a5c813dfc737e377 | Previous audit commit |
| 06.02.2024 | c3f7040960627f023a2098232658c49e74400d03 | Commit for the audit |
| 14.03.2024 | 9e20913fb46db6d3774c56b13ba17d6911cb2caa | Commit for the re-audit |
| 10.05.2024 | c08a3ab8eb29d7622eddf432cb518eeec6f88b63 | Final commit |
| 31.05.2024 | 25fb794f1acea1e1d498fab41f6cab9cbdc565e7 | Commit for the re-audit 2 |
| 29.07.2024 | cd8476a7d80fdc2c61a59f44885cda1fdfbbc4a3 | Commit for the re-audit 3 |
The audit covered the following files:
Ethereum:mainnet
| File name | Contract | Comment |
|---|---|---|
| OneWayLendingFactory.vy | 0xeA6876DDE9e3467564acBeE1Ed5bac88783205E0 | The Factory issues extra approvals to the AMM. This is safe |
| AMM.vy | 0xDf41E21dAe8Bf6Ae3eddb83337f8364Eb7FC1659 | AMM implementation |
| Controller.vy | 0x4c5d4F542765B66154B2E789abd8E69ed4504112 | Controller implementation. Minor changes from the latest commit |
| Vault.vy | 0xc014F34D5Ba10B6799d76b0F5ACdEEe577805085 | Vault implementation. Unnecessary functions were removed from the interface |
| SemilogMonetaryPolicy.vy | 0x4863c6dF17dD59311B7f67E694DD835ADC87f2d3 | Monetary policy implementation |
| CryptoFromPool.vy | 0xC455e6c7936C2382f04306D329ABc5d36444D3F8 | Oracle implementation |
| LiquidityGauge.vy | 0x79D584d2D49eC8CE8Ea379d69364b700bd35874D | Gauge implementation |
| Market 0: wstETH/crvUSD | ||
| AMM.vy | 0x847D7a5e4Aa4b380043B2908C29a92E2e5157E64 | Dynamic fees are not fully implemented. Fixed by setting high fees |
| Controller.vy | 0x1E0165DbD2019441aB7927C018701f3138114D71 | Minor changes |
| Vault.vy | 0x8cf1DE26729cfB7137AF1A6B2a665e099EC319b5 | Unnecessary functions were removed from the interface |
| SemilogMonetaryPolicy.vy | 0x112E37742015ECe4cEB4b576a9434940838eAf02 | wstETH Market |
| CryptoFromPool.vy | 0x5e2406D3D86F8c4a22baC9713F0A38804e9Ef181 | wstETH Market |
| Market 1: WETH/crvUSD | ||
| AMM.vy | 0xb46aDcd1eA7E35C4EB801406C3E76E76e9a46EdF | Dynamic fees are not fully implemented. Fixed by setting high fees. |
| Controller.vy | 0xaade9230AA9161880E13a38C83400d3D1995267b | Minor changes. |
| Vault.vy | 0x5AE28c9197a4a6570216fC7e53E7e0221D7A0FEF | Unnecessary functions were removed from the interface. |
| SemilogMonetaryPolicy.vy | 0xa6c73DC07E17Feda6925C2a4F44C166Fc18Fcf1F | |
| CryptoFromPool.vy | 0x6530B69479549BD3Cc806463964d58D69c285BD8 | |
| Market 2: tBTC/crvUSD | ||
| AMM.vy | 0x5338B1bf469651a5951ef618Fb5DeFbffaed7BE9 | Dynamic fees are not fully implemented. Fixed by setting high fees. |
| Controller.vy | 0x413FD2511BAD510947a91f5c6c79EBD8138C29Fc | Minor changes. |
| Vault.vy | 0xb2b23C87a4B6d1b03Ba603F7C3EB9A81fDC0AAC9 | Unnecessary functions were removed from the interface. |
| SemilogMonetaryPolicy.vy | 0xde31c340545c8031843Bff5Eb42640009961aeEF | |
| CryptoFromPool.vy | 0xeF42b6525454dAbA2d73441a5c749c82a942692B | |
| Market 3: CRV/crvUSD | ||
| AMM.vy | 0xafca625321Df8D6A068bDD8F1585d489D2acF11b | Dynamic fees are not fully implemented. Fixed by setting high fees. |
| Controller.vy | 0xEdA215b7666936DEd834f76f3fBC6F323295110A | Minor changes. |
| Vault.vy | 0xCeA18a8752bb7e7817F9AE7565328FE415C0f2cA | Unnecessary functions were removed from the interface. |
| SemilogMonetaryPolicy.vy | 0x8b6527063FbC9c30731D7E57F1DEf08edce57d07 | |
| CryptoFromPool.vy | 0xE0a4C53408f5ACf3246c83b9b8bD8d36D5ee38B8 | |
| Market 4: CRV/crvUSD | ||
| AMM.vy | 0xe7B1c8cfC0Bc45957320895aA06884d516DAA8e6 | Dynamic fees are not fully implemented. Fixed by setting high fees. |
| Controller.vy | 0xC510d73Ad34BeDECa8978B6914461aA7b50CF3Fc | Minor changes. |
| Vault.vy | 0x4D2f44B0369f3C20c3d670D2C26b048985598450 | Unnecessary functions were removed from the interface. |
| SemilogMonetaryPolicy.vy | 0x40A442F8CBFd125a762b55F76D9Dba66F84Dd6DD | |
| CryptoFromPool.vy | 0xD4Dc9D7567F76fAD2b5A90D3a1bdb3eE801435A8 | |
| Market 5: WETH/crvUSD | ||
| AMM.vy | 0x08Ba6D7c10d1A7850aE938543bfbEA7C0240F9Cf | Dynamic fees are not fully implemented. Fixed by setting high fees. |
| Controller.vy | 0xa5D9137d2A1Ee912469d911A8E74B6c77503bac8 | Minor changes. |
| Vault.vy | 0x46196C004de85c7a75C8b1bB9d54Afb0f8654A45 | Unnecessary functions were removed from the interface. |
| SemilogMonetaryPolicy.vy | 0xbDb065458d34DB77d1fB2862D367edd8275f8352 | |
| CryptoFromPool.vy | 0x4f4B897871902d05cBa110B8e892498f12a20443 | |
| Market 6: tBTC/crvUSD | ||
| AMM.vy | 0xfcb53ED72dAB68091aA6a2aB68b5116639ED8805 | Dynamic fees are not fully implemented. Fixed by setting high fees. |
| Controller.vy | 0xe438658874b0acf4D81c24172E137F0eE00621b8 | Minor changes. |
| Vault.vy | 0x99Cff9Dc26A44dc2496B4448ebE415b5E894bd30 | Unnecessary functions were removed from the interface. |
| SemilogMonetaryPolicy.vy | 0x62cD08caDABF473315D8953995DE0Dc0928b7D3C | |
| CryptoFromPool.vy | 0x33A95b2121fd4eC89D9fA6C5FD21545270bC06a6 | |
| Market 7: sUSDe/crvUSD | ||
| AMM.vy | 0x9bBdb1b160B48C48efCe260aaEa4505b1aDE8f4B | Dynamic fees are not fully implemented. Fixed by setting high fees. |
| Controller.vy | 0x98Fc283d6636f6DCFf5a817A00Ac69A3ADd96907 | Minor changes. |
| Vault.vy | 0x52096539ed1391CB50C6b9e4Fd18aFd2438ED23b | Unnecessary functions were removed from the interface. |
| SemilogMonetaryPolicy.vy | 0xF82A5a3c69cA11601C9aD4A351A75857bDd1365F | |
| CryptoFromPool.vy | 0x50c39EA8f3D72310C8B56A56B333994266e9b477 | |
| Market 8: UwU/crvUSD | ||
| AMM.vy | 0x6BE658242b769500f27498Ba0637406E417507b1 | |
| Controller.vy | 0x09dBDEB3b301A4753589Ac6dF8A178C7716ce16B | Old commit, minor changes |
| Vault.vy | 0x7586C58bf6292B3C9DeFC8333fc757d6c5dA0f7E | Unnecessary functions were removed from the interface. |
| SemilogMonetaryPolicy.vy | 0x9058237c94551770BbB58b710E23e5277b6837da | |
| CryptoFromPool.vy | 0xBcda2aCfE820c92B8E0b389733AaaAE4f930A9F1 | |
| Market 9: WBTC/crvUSD | ||
| AMM.vy | 0x8eeDE294459EFaFf55d580bc95C98306Ab03F0C8 | |
| Controller.vy | 0xcaD85b7fe52B1939DCEebEe9bCf0b2a5Aa0cE617 | Old commit, minor changes |
| Vault.vy | 0xccd37EB6374Ae5b1f0b85ac97eFf14770e0D0063 | Unnecessary functions were removed from the interface. |
| SemilogMonetaryPolicy.vy | 0xE53C7e8857fAd35dF5a02a2899CaF5871A50bA95 | |
| CryptoFromPool.vy | 0xE3ee57D3fbdBAE9b506a92da79Ca8454AC288E2C | |
| Market 10: pufETH/crvUSD | ||
| AMM.vy | 0xcd28cF8f7755f03967D27E128B38022B63919836 | |
| Controller.vy | 0x4f87158350c296955966059C50263F711cE0817C | Old commit, minor changes |
| Vault.vy | 0xff467c6E827ebbEa64DA1ab0425021E6c89Fbe0d | Unnecessary functions were removed from the interface. |
| SemilogMonetaryPolicy.vy | 0x2e478Df674e25b2724B1aC9BCABa644943dA359F | |
| CryptoFromPool.vy | 0xb08eB288C57a37bC82238168ad96e15975602cd9 |
Arbitrum:mainnet
| Severity | # of Findings |
|---|---|
| CRITICAL | 1 |
| HIGH | 2 |
| MEDIUM | 5 |
| LOW | 7 |
In this audit, we have examined various security and functionality aspects to ensure the robustness and reliability of the system. Our primary focus was on the lending features built on the existing codebase of the Curve stablecoin, particularly the interactions between vaults in the TwoWayLendingFactory and the broader implications of permissionless pool deployment. Based on our findings, we offer several suggestions to enhance the project's security posture and user experience.
Key Audit Vectors:
- Reentrancy Attacks: We checked for reentrancy attacks, especially when combining two pools within the TwoWayLendingFactory. Thanks to the implementation of
check_lock()mechanisms and non-reentrant flags, we found no vulnerabilities in this area. - Vault Share Attacks and Inflation: We analyzed potential attacks on vault shares, including inflation attacks and rounding errors, ensuring the integrity of asset valuation within the system.
- Oracle Price Manipulation: Our examination extended to potential manipulations of oracle prices, particularly the impact on share prices in two-way lending and issues arising from price volatility.
- Interest Rate Accrual Accuracy: We verified the correctness of interest rate accruals crucial for maintaining fair and predictable lending and borrowing conditions.
- Factory Invariants and Common Issues: Attempts were made to disrupt or exploit factory invariants, alongside a comprehensive check for common security issues prevalent in DeFi protocols.
Recommendations:
- Enhanced Documentation for Users: Given the permissionless nature of pool deployment, it is vital to improve documentation for users. Clarifications on the risks associated with the delay between the EMA Oracle and instant prices, especially in scenarios where a token's price might surge by 20% leading to potential pool insolvency, would be beneficial. Including simulation examples that detail these parameters and warnings against using unconventional tokens (e.g., tokens with fees, ERC-777, or rebaseable tokens) could significantly enhance user understanding and safety.
- Code Comment Revisions: Some code comments require updates for clarity, such as specifying that the
use_ethparameter in Controller is now unused.
Notes on Fixes
The final commit includes two fixes aimed at addressing the issue of hard liquidations in cases of oracle manipulation.
To address the issue of manipulations through Inflation Attack for some oracles, a limit on the rate of price growth was introduced. It should be noted that such a limit will create a lag between the real and actual price of the asset. Therefore additional analysis is needed to ensure that this lag does not affect the delay in the PegKeeper's operation. If the PegKeeper is late in normalizing the price, some borrowers may face unfair liquidations.
To protect against two-block market manipulations, special dynamic fees were introduced, making the attack more costly. It should be noted that for some older AMMs, this feature was not implemented, so fees were manually increased as a hotfix. Dynamic and increased fees do not fully protect against manipulations but they significantly raise the cost of a two-block attack and make this vector even less realistic.
Another note concerns the incorrect operation of the remove_price_pair() function: since old contracts are not upgradeable, a workaround is expected to be used. One can remove several items from the end of the list and then add them back again without the one to be removed, all in a single transaction.
Fixed in https://github.com/curvefi/curve-stablecoin/commit/9e20913fb46db6d3774c56b13ba17d6911cb2caa
In TwoWayLending, the share price can easily inflate through a direct transfer of funds to the Controller. This can be used to trigger hard liquidations:
- A hacker buys the victim's collateral shares in the
AMMusingexchange(). - The hacker inflates the collateral price per share (even a small increase of +1.1% may be sufficient).
- The victim's health becomes negative, and the hacker can liquidate it for profit.
Multiple tests show that the attack have been forwarded to the client.
In order to understand why it works, we'll have to take two features of the protocol into consideration.
Feature 1: If the oracle price increases by N percent, then the price of the tick p_current_down() increases by ~3.3 times.
That is, for example, if the price in the oracle increased by 10%, then the price grid in the AMM shifted not by 10%, but on average somewhere around 33%.
Feature 2: The AMM uses the limit_p_o() function which limits price surges and increases fees when the oracle price fluctuates.
For example, if the oracle price increases by 10% in a transaction (for example, from 1 to 1.1), then in the same transaction, limit_p_o() will return 1.1 (small fluctuations are not thresholded), and the dynamic fee will be high.
Over time, approximately after 120 seconds, the dynamic fee will drop to its minimum value.
Example: Let's see what happens after these operations are performed:
- A user creates a loan with
create_loan(). - A hacker buys all collateral in the
AMMwithexchange(). - The oracle price inflates!
As soon as the oracle price increases, say by 10%, the price in ticks increases by 33%. However, if the hacker decides to buy back their stablecoins right after this, they will, surprisingly, spend more collateral than they initially bought. It's because of the dynamic commission returned by limit_p_o(). It's large and compensates for the tick price increase.
However, after 120 seconds the commission drops to the minimum, and after that the hacker can buy back their stablecoins not just at market price, but at a much more favorable price (because the tick price increases by 3.3 times compared to the oracle).
Herein lie two problems.
Problem 1: The health() function does not account for dynamic fees and, therefore, in this scenario, is negative. In reality, the user's position remains healthy, at least while the dynamic fees are high, because if the hacker makes the reverse trade, they would add even more collateral to the user than was initially present. The get_x_down() function does not take this into account.
In particular, because of this, a problem arises that we can now hard liquidate the user and extract profit. Whereas for the hard liquidation, we don't need to make the reverse trade and spend the dynamic fee.
Problem 2: As the tick price grows three times faster than the oracle, bad debt may accumulate.
Suppose:
- We brought 100 collateral and borrowed 88 stablecoins.
- We buy back our collateral for 105 stablecoins.
- Suppose then the price increases by 10%.
- 120 seconds pass for the
limit_p_o()fees to fall.
How much of the collateral can we sell back in the AMM to retrieve our stables? The collateral now costs 10% more on the market, but we can sell it at a price 33% higher in the AMM! That is, we buy back our 105 stablecoins for about 75 collateral.
But on the market, 75 collateral are worth 75*1.10 = 82 stablecoins?
Thus, it turns out we borrowed 88 stablecoins in the AMM, but then put back collateral that is worth 82 stablecoins, which is less.
In conclusion, the fact that the price in ticks changes by 3.3 times more than the oracle, combined with the user's health not accounting for dynamic fees, and the ability of a hacker to inflate the price of collateral (which are simply vault shares in the case of TwoWayLending), enables the hacker to profitably liquidate users whose positions should otherwise be considered healthy.
We recommend revisiting the logic of the health() function, not relying on balanceOf() while calculating pricePerShare() in vault's shares, and thinking of a way to make it impossible for a hacker to perform attacks such as exchange(forth)+inflate+liquidate() or exchange(forth)+inflate+exchange(back), as the current implementation of the protocol, specifically the tick price rising three times higher than the oracle, is highly susceptible to such attacks.
- Added dynamic fee which fixes very related problem - 2-block attacks with ANY market in a similar fashion
- Limited growth of pricePerShare to 1% a minute in 0c373156fb58ae89b6a8234d6ed3ff82eda82d4f
Fixed in https://github.com/curvefi/curve-stablecoin/commit/9e20913fb46db6d3774c56b13ba17d6911cb2caa
The admin_fees_x variable is not divided by BORROWED_PRECISION in AMM withdraw():
# If withdrawal is the last one - transfer dust to admin fees
if new_shares == 0:
if x > 0:
self.admin_fees_x += x
if y > 0:
self.admin_fees_y += unsafe_div(y, COLLATERAL_PRECISION)
If borrowed_token is WBTC (decimals=8), then the error in fee accrual will be on the order of 10 magnitudes. A hacker could perform an inflation attack on the nearest available tick to trigger this piece of code and inflate admin_fees_x to the total amount of the borrowed_token balance in the AMM, while losing 10 magnitudes less funds than the final inflation amount. If the hacker then calls collect_fees(), which is a public method, all borrowed tokens from the AMM will be sent to FACTORY.fee_receiver().
This will lead to a partial DOS of the AMM, as users will lose the ability to withdraw borrowed tokens from ticks, as there simply won't be any funds in the AMM for this.
There are a few notes on this:
- This attack does not depend on
ADMIN_FEE; the code is always activated when there is dust in the tick. In order to execute the attack, the hacker needs to inflate the share price in the tick, causing the dust to have a large value. - AMM uses dead shares, but price inflation is still possible via the
exchange()method or other means. It's just not profitable for the hacker. - Currently,
collect_fees()reverts asVaultdoes not have afee_receiver()method which is called by thecollect_fees()method. Still, if thecollect_fees()revert issue is addressed by introducing afee_receiver()in the factory, then the fee inflation bug will arise.
We recommend adding the missing division by the BORROWED_PRECISION:
self.admin_fees_x += unsafe_div(x, BORROWED_PRECISION)
Fixed in 3336ed838f8ba90490155d7401c4c4eb96824b5c
Fixed in https://github.com/curvefi/curve-stablecoin/commit/cd8476a7d80fdc2c61a59f44885cda1fdfbbc4a3
The AggregateStablePrice contract doesn't remove the corresponding entry in the last_tvl array during the remove_price_pair() call.
This may result in inaccurate TVL calculations and incorrect price aggregations.
We recommend adjusting the remove_price_pair() function to remove the corresponding entry in the last_tvl array when a price pair is removed.
Old contracts are not upgradeable, thus, a workaround will be used: one can remove several items from the end of the list and then add them back again without the one to be removed, all in a single transaction.
Acknowledged
Lending factories have public deploy_gauge() to deploy a gauge for an chosen vault:
- https://github.com/curvefi/curve-stablecoin/blob/c3f7040960627f023a2098232658c49e74400d03/contracts/lending/OneWayLendingFactory.vy#L326-L343
- https://github.com/curvefi/curve-stablecoin/blob/c3f7040960627f023a2098232658c49e74400d03/contracts/lending/TwoWayLendingFactory.vy#L371-L388
There are two problems there:
- One gauge per vault. Only the first
deploy_gauge()caller can create a gauge for a vault. - The caller of
deploy_gauge()receives rights in the created gauge. This caller as amanagercan add 8 malicious reward tokens throughadd_reward().
These two things together will permanently block adding new valid reward tokens and creating the correct gauge (even if a manager role is reset).
Consider a few options:
- Allow multiple gauges for every vault and not revert if a gauge for a vault already exists. It will allow ignoring malicious gauges.
- Create a gauge for a vault right after vault deployment inside
_create(). - Ensure that users can create vaults and deploy gauges atomically in one transaction.
This is a known issue (or nonissue) in ALL the gauges. If ever is a problem - can create in a single tx
Acknowledged
Liquidity gauge manager is set as a tx.origin of the gauge deploy transaction.
It is not recommended to have tx.origin in access control logic.
Some DeFi users are Multisigs or Account Abstraction wallets.
In such cases, tx.origin is not correct final user identification.
We recommend having manager as a customizable input for factory.deploy_gauge().
This is a known issue (or nonissue) in ALL the gauges.
Fixed in https://github.com/curvefi/curve-stablecoin/commit/9e20913fb46db6d3774c56b13ba17d6911cb2caa
The TwoWayLendingFactory.create_from_pool method always returns an error with CryptoFromPoolVault. It happens so because vault.borrowed_token() was not initialised at the time of validation:
assert pool.coins(collateral_ix) == vault.borrowed_token()
We recommend passing an initialised Vault to the CryptoFromPoolVault or moving the check to Factory.
Indeed, and there is enough validation in the factory already regardless -> removing in 069b3886f90cb49903ab1bf0e1af80fd70ead710.
Fixed in https://github.com/curvefi/curve-stablecoin/commit/9e20913fb46db6d3774c56b13ba17d6911cb2caa
- https://github.com/curvefi/curve-stablecoin/blob/c3f7040960627f023a2098232658c49e74400d03/contracts/price_oracles/CryptoFromPoolVault.vy#L65
- https://github.com/curvefi/curve-stablecoin/blob/c3f7040960627f023a2098232658c49e74400d03/contracts/price_oracles/OracleVaultWrapper.vy#L39
These oracles are used in TwoWayLendingFactory. If you try to influence the price via controller_long/controller_short, there will be a check_lock check (https://github.com/curvefi/curve-stablecoin/blob/c3f7040960627f023a2098232658c49e74400d03/contracts/lending/Vault.vy#L266).
However, an attacker still has the ability to influence the price via a transfer to the controller. For example:
print(amm_short.price_oracle()) # 1000000000000000
borrowed_token.transfer(controller_long, amount)
print(amm_short.price_oracle()) # 1250000000000000
This can be advantageous if there are few funds in the Controller and it is very easy to influence the price.
We recommend using a more stable oracle to price the LP Vault.
Limited growth of pricePerShare to 1% a minute in 0c373156fb58ae89b6a8234d6ed3ff82eda82d4f.
Acknowledged
If the price_w() function, which updates last_timestamp, is not called for a long time, the value of alpha decreases, leading to a risk of EMA manipulation by a hacker.
This occurs because the closer alpha gets to zero, the greater the influence of the new totalSupply() value, which can be manipulated within the current transaction:
alpha: uint256 = 10**18
if last_timestamp < block.timestamp:
alpha = self.exp(- convert((block.timestamp - last_timestamp)
* 10**18 / TVL_MA_TIME, int256))
...
if alpha != 10**18:
# alpha = 1.0 when dt = 0
# alpha = 0.0 when dt = inf
new_tvl: uint256 = self.price_pairs[i].pool.totalSupply()
tvl = (new_tvl * (10**18 - alpha) + tvl * alpha) / 10**18For example, after 10 * TVL_MA_TIME seconds (which is about 5 days in the current implementation), if no one calls the function, alpha becomes 0.0000453999. Ultimately, if alpha=0, new_tvl will simply be equal to totalSupply().
We recommend considering the possibility of manipulation with this aggregator and, for example, implementing monitoring to check if the price_w() function has not been called for a long time.
This is indeed true. However, MA time is really large, and the same aggregator is used for multiple markets. So price_w is called really often. Nevertheless, not a bad idea to do the activity checking, as recommended.
Acknowledged
The EMA does not follow the market instantly, resulting in a delay between the actual price and the oracle in Curve. Hypothetically, a situation could arise where the market price jumps so significantly that it becomes profitable to borrow this token at a lower price in the Curve lending and sell it at a higher price on the market. This can be done in a flash loan, instantly deleting the Vault and leaving the protocol with bad debt.
We recommend disallowing borrowing (using create_loan()) if the momentary price significantly differs from the EMA.
This is a design decision and fundamental to the algorithm.
Acknowledged
Chainlink feed data have edge cases that are recommended being covered. The current issues are:
- Stale thresholds are not implemented in some oracles (
CryptoWithStablePriceAndChainlink,CryptoWithStablePriceAndChainlinkFrxeth); - Stale threshold is 24 hours now. But the Chainlink price synchronizes for low volatile pairs like stETH/ETH can update with the stale threshold slightly above 24 hours. In this case, the feed is not stale;
updateTime != 0is not checked (it means that the round is not complete);answeredInRound >= roundIdis not checked (it can additionally indicate that the price is stale).
Consider the following improvements:
- Implement stale price checks for
CryptoWithStablePriceAndChainlinkandCryptoWithStablePriceAndChainlinkFrxeth - Consider updating
CHAINLINK_STALE_THRESHOLDto 24.5 hours in cases of low volatile feeds - Require that
updateTime != 0 - Require that
answeredInRound >= roundId
That is absolutely true, however using chainlink introduce other issues, so we don't use it. It is, however, possible to make EMA oracle out of chainlink oracles. Gas costs for that are prohibitive on Ethereum mainnet, but totally acceptable on L2s
Acknowledged
There is currently no way to change price_oracle in the AMM.
price_oracle_contract: public(immutable(PriceOracle))
AMM.price_oracle is external, it may be worth keeping the ability to change it.
This is correct and design decision to not have the admin (even DAO) be able to rug everything
Acknowledged
EMAPriceOracle is a wrapper that adds an Exponential Moving Average (EMA) to another price source.
When it is deployed or the protocol has not been used for some time, the EMA prefers new values from the price source over old ones:
alpha: uint256 = self.exp(- convert((block.timestamp - last_timestamp)
* 10**18 / MA_EXP_TIME, int256))
return (current_price * (10**18 - alpha) + last_price * alpha) / 10**18
At such times, a hacker can shift the price from the source for just one second and record a new price value in EMAPriceOracle. Although the real price source will return to its original value immediately, the EMAPriceOracle will need about 4 * MA_EXP_TIME seconds to catch up. Until then, it will give a deviated price, which could be used for arbitrage and creating bad debt.
We recommend preferring old values over new ones when calculating the EMA.
Wrapper is not to be used: now we use "native" EMA oracle in pools which is not affected by such manipulations.
Fixed in https://github.com/curvefi/curve-stablecoin/commit/c08a3ab8eb29d7622eddf432cb518eeec6f88b63
- https://github.com/curvefi/curve-stablecoin/blob/9e20913fb46db6d3774c56b13ba17d6911cb2caa/contracts/lending/TwoWayLendingFactory.vy#L589
- https://github.com/curvefi/curve-stablecoin/blob/9e20913fb46db6d3774c56b13ba17d6911cb2caa/contracts/lending/TwoWayLendingFactory.vy#L612
It is possible to cause a temporary DOS in the exhange() and exchange_dy() functions in TwoWayLendingFactory by directly transferring tokens to the contract.
For example, the following line reverts if the right side of subtraction is higher:
dxy[0] = max_in - self.transfer_out(vault, other_vault, i, receiver)
The attack scenario:
Suppose a user calls the function:
TwoWayLendingFactory.exchange_dy(vault_id, i=1, j=0, amount, max_in, receiver).
Under normal operation, an amount of collateral tokens will be deducted from the user, converted into shares:
_max_in: uint256 = self.transfer_in(
vault, other_vault, i=1, msg.sender, max_in
)
assert token.transferFrom(_from, self, amount,
default_return_value=True)
...
return other_vault.deposit(amount)
Then exchanged for borrowed token in favor of the receiver:
dxy: uint256[2] = self.amms[vault_id].exchange_dy(
i, j, _amount, _max_in, _receiver
)
And, finally, all the remaining unspent collateral will be returned:
dxy[0] = max_in - self.transfer_out(vault, other_vault, i=1, receiver)
In our case, the transfer_out() function will return:
other_vault.redeem(other_vault.balanceOf(self), _to)
The exploiter sandwiches the transaction:
- Before the user's transaction, they deposit an extra amount of shares into TwoWayLendingFactory.
- Then the user's transaction is executed, but it reverts at this line because the contract's balance now contains not only the user's trade remainder but also the extra amount of shares deposited by the exploiter:
max_in - self.transfer_out(vault, other_vault, i=1, receiver)
- Afterwards, the exploiter withdraws their funds using an empty trade with
max_in=infinity.
Thus, the exploiter can always revert the trader's transaction. No one loses money (except for gas).
Also, there's another attack variant without sandwiching: the exploiter simply deposits different tokens into TwoWayLendingFactory, say, worth $1-100. This reverts all trades with a small slippage, until a trade with a big possible slippage occurs.
We recommend taking into account a scenario where funds are already available on the contract before any trades.
Acknowledged
Rates are excluded from future use if stored_rates() returns 10**18 during initialization:
u: bool = False
for r in stored_rates:
if r != 10**18:
u = True
use_rates.append(u)
This may be incorrect if stored_rates() changes over time.
We recommend using all rates, as they may deviate from 10**18 over time.
It is assumed that the deployer validates the oracle after deployment
Acknowledged
If the sequencer is down and the difference block.timestamp - self.cached_timestamp gets too high, the protection against inflation attacks using RATE_MAX_SPEED may be broken.
We recommend taking into account the case of a non-functioning sequencer when using RATE_MAX_SPEED.
Good point, needs to be addressed
MixBytes is a team of blockchain developers, auditors and analysts keen on decentralized systems. We build opensource solutions, smart contracts and blockchain protocols, perform security audits, work on benchmarking and software testing solutions, do research and tech consultancy.