Platform: Code4rena
Start Date: 08/05/2023
Pot Size: $90,500 USDC
Total HM: 17
Participants: 102
Period: 7 days
Judge: 0xean
Total Solo HM: 4
Id: 236
League: ETH
Rank: 5/102
Findings: 3
Award: $3,352.89
🌟 Selected for report: 2
🚀 Solo Findings: 0
🌟 Selected for report: xuwinnie
Also found by: BoltzmannBrain, Udsen, mussucal
951.5947 USDC - $951.59
Functon liquidateAccount will fail if the transaction is not included in current block because interest accures per block, and repayAmount and borrowBalance need to match precisely.
At the end of function liquidateAccount, a check is performed to ensure that the borrowBalance is zero:
for (uint256 i; i < marketsCount; ++i) { (, uint256 borrowBalance, ) = _safeGetAccountSnapshot(borrowMarkets[i], borrower); require(borrowBalance == 0, "Nonzero borrow balance after liquidation"); }
This means that repayAmount specified in calldata must exactly match the borrowBalance. (If repayAmount is greater than borrowBalance, Comptroller.preLiquidateHook will revert with error TooMuchRepay.) However, the borrowBalance is updated every block due to interest accrual. The liquidator cannot be certain that their transaction will be included in the current block or in a future block. This uncertainty significantly increases the likelihood of liquidation failure.
Manual
Use a looser check
snapshot = _getCurrentLiquiditySnapshot(borrower, _getLiquidationThreshold); require (snapshot.shortfall == 0);
to replace
for (uint256 i; i < marketsCount; ++i) { (, uint256 borrowBalance, ) = _safeGetAccountSnapshot(borrowMarkets[i], borrower); require(borrowBalance == 0, "Nonzero borrow balance after liquidation"); }
Invalid Validation
#0 - c4-judge
2023-05-17T20:50:44Z
0xean marked the issue as primary issue
#1 - c4-sponsor
2023-05-23T21:26:07Z
chechu marked the issue as disagree with severity
#2 - chechu
2023-05-23T21:26:11Z
Suggestion: QA (no risk for funds, no risk of DoS)
Liquidator has to take into account the pending interests to be accrued before invoking liquidateAccount. It’s technically feasible, and if the TX fails they can retry it, so finally the position will be liquidated. The amount to be liquidated will be very low, so we don’t see any risk of front running.
#3 - 0xean
2023-05-31T00:08:54Z
They would have to know which block specifically their transaction would get mined in to be able to precompute this.
While they could retry the transaction, I do think this will have an impact of the protocol's availability under normal conditions and therefore does meet the criteria for Medium severity.
#4 - c4-judge
2023-06-05T14:11:44Z
0xean changed the severity to 2 (Med Risk)
#5 - c4-judge
2023-06-05T14:11:56Z
0xean marked the issue as satisfactory
#6 - c4-judge
2023-06-05T16:54:21Z
0xean marked the issue as selected for report
#7 - Nabeel-javaid
2023-06-06T06:33:14Z
Both the judge and sponsor comments show this is either low or QA but marked as medium.
#8 - 0xean
2023-06-06T12:56:21Z
@Nabeel-javaid - please re-read my comment https://github.com/code-423n4/2023-05-venus-findings/issues/365#issuecomment-1569305443
#9 - chechu
2023-06-08T10:32:39Z
They would have to know which block specifically their transaction would get mined in to be able to precompute this.
While they could retry the transaction, I do think this will have an impact of the protocol's availability under normal conditions and therefore does meet the criteria for Medium severity.
The liquidator can calculate the exact needed amount in a contract, for example, to guarantee that the amount is valid in the same block where the transaction will be minted.
@0xean
#10 - 0xean
2023-06-08T12:17:54Z
@chechu - I agree, that is possible, but how does one do this from an EOA?
#11 - chechu
2023-06-08T13:54:31Z
@chechu - I agree, that is possible, but how does one do this from an EOA?
@0xean with an EOA I would do a multicall, first statically invoking the functions to accrue interest, and then invoking the function to get the precise borrow amounts. You could use https://www.npmjs.com/package/ethereum-multicall, for example, to do this. We do something similar in our frontend here: https://github.com/VenusProtocol/venus-protocol-interface/blob/develop/src/clients/api/queries/getVaiCalculateRepayAmount/index.ts#L17-L33
By doing this, it's true that your TX maybe is not included in the current block and it won't be valid in the next one.
#12 - 0xean
2023-06-08T14:07:32Z
Yea, this seems like a workaround to the problem in my opinion. Why would you be opposed to simply updating the function to make it more tolerant to the specific block its called in?
2349.6167 USDC - $2,349.62
The function _ensureMaxLoops reverts if the iteration count exceeds the maxLoopsLimit. However, the limitation imposed by maxLoopsLimit hinders the functioning of liquidateAccount under certain conditions, as orderCount needs to reach twice the market count (which is also constrained by the maxLoopsLimit) in extreme cases.
Suppose maxLoopsLimit is set to 16 and currently 12 markets has been added, which is allowed by _ensureMaxLoops in function _addMarket:
allMarkets.push(VToken(vToken)); marketsCount = allMarkets.length; _ensureMaxLoops(marketsCount);
Then, Alice enters all the 12 markets by depositing and borrowing simultaneously, which is also allowed by _ensureMaxLoops in function enterMarkets:
uint256 len = vTokens.length; uint256 accountAssetsLen = accountAssets[msg.sender].length; _ensureMaxLoops(accountAssetsLen + len);
To illustrate, assume these 12 coins are all stablecoin with an equal value. Let's call them USDA, USDB, USDC,..., USDL. Alice deposits 20 USDA, 1.1USDB, 1.1USDC,..., 1.1USDL, worth 32.1USD in total, then she borrows 2USDA, 2USDB, 2USDC,..., 2USDL, worth 24 USD in total. Unluckily, USDA depegs to 0.6USD, Alice's deposit value drop to 24.1USD, which is below the liquidation threshold (also below the minLiquidatableCollateral). However, nobody can liquidate Alice's account by calling liquidateAccount, because the least possible orderCount is 23, which exceeds maxLoopsLimit.
Let's take a closer look at LiquidationOrder:
struct LiquidationOrder { VToken vTokenCollateral; VToken vTokenBorrowed; uint256 repayAmount; }
In this case, liquidator cannot perfectly match vTokenCollateral with vTokenBorrowed one-to-one. Because the value of collateral and debt is not equal, more than one order is needed to liquidate each asset. To generalize, if asset count is n, in the worst case, 2n-1 orders are needed for a complete liquidation (not hard to prove).
Manual
_ensureMaxLoops(ordersCount / 2);
Loop
#0 - 0xean
2023-05-18T10:53:23Z
more than one order is needed to liquidate each asset
I am not sure I am tracking this assertion in the above report.
#1 - c4-judge
2023-05-18T11:44:28Z
0xean marked the issue as primary issue
#2 - c4-sponsor
2023-05-23T21:24:51Z
chechu marked the issue as sponsor disputed
#3 - chechu
2023-05-23T21:25:09Z
We can resolve it by just increasing the limit accepted by _ensureMaxLoops, with a VIP. BTW, the suggestion is not valid, because the number of iterations will be ordersCount, not ordersCount/2
#4 - 0xean
2023-05-30T22:33:58Z
We can resolve it by just increasing the limit accepted by _ensureMaxLoops, with a VIP.
i don't think this is a valid mitigation and would still cause a disruption to the protocol that warrants the M severity.
Per the c4 docs
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
#5 - 0xean
2023-05-30T22:37:54Z
@chechu - the validity of this issue comes down to the assumption of there being more than 1 order per asset, lets discuss that point specifically, because otherwise this issue doesn't seem to be valid.
#6 - 0xean
2023-06-02T14:19:11Z
@chechu this is the last issue I could use your response on. Thank you for all the great responses thus far.
#7 - chechu
2023-06-04T15:16:32Z
Sorry for the delay in my response. Reviewing it more carefully, the finding seems valid. It's true that in Comptroller.liquidateAccount several orders per borrowed asset could be needed. Example:
Params:
Borrower position:
Every condition is satisfied to allow the execution of liquidationAccount:
collateralToSeize ($35 * 1.1 = $38.5) < total collateral ($40)But the liquidator cannot repay $35 of the borrowed asset and get enough tokens of just one of the collaterals. So, the liquidator will need two orders:
This way, the final position of the borrower will be:
Comptroller.liquidateAccount, and it would be impossible to get only using one of the available collateralsWe will apply the mitigation suggested by the warden
#8 - c4-sponsor
2023-06-04T15:16:37Z
chechu marked the issue as sponsor confirmed
#9 - c4-judge
2023-06-05T14:10:18Z
0xean marked the issue as satisfactory
#10 - c4-judge
2023-06-05T16:59:59Z
0xean marked the issue as selected for report
51.6843 USDC - $51.68
When a new market is added with totalSupply equals to zero, Attacker can exploit a rounding error to drain the pool. This vulnerability has been used to attack Hundred Finance and influences all Compound v2 forks.
In function redeemUnderlying,
redeemTokens = div_(redeemAmountIn, exchangeRate); redeemAmount = redeemAmountIn;
Attacker donates to the pool to inflate exchangeRate (zero previous supply makes this easy), then redeemUnderlying is called, the calculated redeemTokens value is around 1.999 but rounded to 1. Thanks to this, Comptroller.preRedeemHook passes and attacker can sucessfully redeem the collateral (without repaying the debt first).
Detail can be found at https://blog.hundred.finance/15-04-23-hundred-finance-hack-post-mortem-d895b618cf33
redeemTokens = div_(redeemAmountIn, exchangeRate) + 1;
Math
#0 - c4-judge
2023-05-17T17:57:45Z
0xean marked the issue as duplicate of #314
#1 - c4-judge
2023-06-05T14:08:37Z
0xean marked the issue as satisfactory
#2 - c4-judge
2023-06-05T14:37:43Z
0xean changed the severity to 2 (Med Risk)