code-423n4 / 2023-05-venus-findings

2 stars 1 forks source link

`Comptroller.sol#_getHypotheticalLiquiditySnapshot` assumes that all UnderlyingTokens have the same precision #448

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L1331

Vulnerability details

Impact

File: Comptroller.sol
1316             // Get the normalized price of the asset
1317             Exp memory oraclePrice = Exp({ mantissa: _safeGetUnderlyingPrice(asset) });
1318
1319             // Pre-compute conversion factors from vTokens -> usd
1320             Exp memory vTokenPrice = mul_(Exp({ mantissa: exchangeRateMantissa }), oraclePrice);
1321             Exp memory weightedVTokenPrice = mul_(weight(asset), vTokenPrice);
1322
1323             // weightedCollateral += weightedVTokenPrice * vTokenBalance
1324             snapshot.weightedCollateral = mul_ScalarTruncateAddUInt(
1325                 weightedVTokenPrice,
1326                 vTokenBalance,
1327                 snapshot.weightedCollateral
1328             );
1329
1330             // totalCollateral += vTokenPrice * vTokenBalance
1331             snapshot.totalCollateral = mul_ScalarTruncateAddUInt(vTokenPrice, vTokenBalance, snapshot.totalCollateral);

Suppose exchangeRate = uTokenBalance / vTokenBalance

uTokenPrice = oraclePrice (decimal always is 6) https://docs.compound.finance/v2/prices/

vTokenPrice = exchangeRate * uTokenPrice 

Collateral = vTokenPrice * vTokenBalance
  = exchangeRate  * uTokenPrice * vTokenBalance
  = uTokenBalance / vTokenBalance * uTokenPrice * vTokenBalance
  = uTokenBalance * uTokenPrice
  = uTokenBalance * oraclePrice

Therefore, the precision of Collateral is the same as that of UnderlyingToken, and the precision of each UnderlyingTokens may be different. Direct addition will lose some UnderlyingTokens with less precision

Proof of Concept

Attackers can exploit this issue to liquidate healthy positions

Tools Used

manual

Recommended Mitigation Steps

Consider the case where UnderlyingTokens have different precision

Assessed type

Decimal

c4-judge commented 1 year ago

0xean marked the issue as unsatisfactory: Insufficient proof