Curve read-only reentrancy in a few sentences
The vulnerable function in the Curve pools is `get_virtual_price`. Silo's Curve LP token markets use this function in the CurvePAPTokensPriceProvider (PAP — pegged assets pools) to calculate the price of the LP tokens.
A possible scenario for the attack is:
Take a flash-loan
Add liquidity into the pool
Remove liquidity. In this step — pools with Ether or ERC-667 transferAndCall, ERC777TokensRecipient, _afterTokenTransfer, _beforeTokenTransfer, or any notification to the sender or recipient will send a notification about the transfer (or do a call in the case with Ether) to the attacker.
Receive a call from the Curve pool and go to the Silo borrow tokens. In this step, the Curve invariant is broken by temporarily creating inconsistencies between D and the total LP token supply (see more details in the ChainSecurity post). So, the function `get_virtual_price` will return an invalid price.
Each protocol that integrates with the vulnerable pool has to implement its solution.
Pools analysis as not all pools are vulnerable
If the Curve pool does not work with raw Ether (ETH) and all underlying tokens have no hooks on the transferFrom function, we can consider it a non-reentrant pool that doesn’t need protection from the read-only reentrancy attack.
Prior to launching Curve LP token silos, the Silo core team conducted a review of the Curve pools. Based on our analysis, we grouped Curve pools into five groups: - Group 1: vulnerable (ETH-stETH, ETH-cbETH, ETH-alETH, ETH-sETH) - Group 2: non-vulnerable, but one of the underlying assets is a proxy (FRAX-USDC) - Group 3: non-vulnerable metapools, with at least one underlying asset implemented as a proxy in the base pool (BUSD-3CRV, TUSD-3CRV) - Group 4: non-vulnerable metapools, where one underlying asset is implemented as a proxy and with at least one underlying asset implemented as a proxy in the base pool (alUSD-3CRV, alUSD-FRAXBP) - Group 5: non-vulnerable (ETH-USDT-WBTC (tricrypto))
The core team team decided that we have zero trust in the pools with pegged assets where at least one of the underlying assets is implemented as a proxy. Such groups as 2, 3, and 4 are treated as vulnerable.
The solution proposed by the ChainSecurity team
In the “Curve LP Oracle Manipulation: Post Mortem” article, you will find a section: “Fixing price feeds”. Where written: “The solution identified most suitable was calling the pools’ withdraw_admin_fees function to trigger the reentrancy lock. Most protocols followed that protection pattern.”
Basically, it looks simple, call the ‘withdraw_admin_fees’ function, and you are fine. This function should revert in the case of the attack.
What’s wrong with the solution proposed by the ChainSecurity team?
When we started implementation/testing, we discovered that the ‘withdraw_admin_fees’ is not implemented for all pools (for some, it is claim_admin_fees), and for some pools, it is allowed only for the pool owner (we are not able to execute it), and some implementations of the ‘withdraw_admin_fees’ fn can’t protect from the attack. Also, the cost for the execution of the cheapest implementation (there are also different implementations of this function) is 7k of gas in case there are no admin fees and > 70k with fees, which will be added to each call into the any Silo function as all of them read the price, and we have to enable the protection from the read-only reentrancy attack.
Is it possible to do better?
To understand if we are under attack, it is enough to check if the Curve pool was locked by the `@nonreentrant(‘lock’)` modifier. We can do it by executing any other function with the same modifier (which in some implementations is `withdraw_admin_fees` proposed by the ChainSecurity team). But there is one requirement for this function. It should revert only in the case if `@nonreentrant(‘lock’)` was activated.
This requirement significantly narrows the number of functions we can use for this kind of protection and, in some pools, makes it even impossible if we simply call the function. But… Do we need to fully execute the function to determine if the lock is activated? No! Everything that we need is to execute some code that is after the lock verification. If we are able to pass the `lock` check, it means that it is not activated, and we are fine.
How can we do it? By limitation of the gas limit for the external call. Wedid some tests by implementing a smart contract that simulated an attack. We discovered that if the pool is locked, the transaction that reverts when calling any function in the pool with the `@nonreentrant(‘lock’)` modifier consumes ~1500 gas. So, everything that we need to ensure that the pool is not locked is to pass this limit by the gas consumption. For example, we can set a gas limit for the call 3000; if the call consumes 3000 of gas, we know it is NOT locked, and we can be sure we are not under attack. This solution is 2–35 times cheaper than proposed by the ChainSecurity team and will work for ANY Curve pool. We did an analysis of Curve pools functions. we picked the `remove_liquidity` fn for verification. It is present in each pool. The `@nonreentrant(‘lock’)` is allocated at the beginning of the function, and there are no other modifiers.
With this approach to be safe, we’ll need to check a pool lock for the pools that are in Group 1 and Group 2. For the pools that are in Group 3, well need to check if the base pool is locked or not. And for the pools from Group 4, we will need to check if the pool and the base pool are locked.
And what is also important as the solution is not expensive. We don’t need to lock silos if the proxy implementation is updated. Our solution and a test prove that our solution for sure protects from the attack pools with vyper 0.2.8 and all other higher versions. Lover versions of the vyper need to be tested.
Last updated