Foundry DeFi Stablecoin CodeHawks Audit Contest
Cyfrin
DeFiFoundry
15,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Incorrect Decimal assumption can lead to protocol breakdown

warRoom

Summary

Improper decimal precision handling for the non 18 decimal ERC20 token can result in DOS for the many user and their collateral being locked forever

Vulnerability Details

Protocol is design to work perfectly for the 18-decimal ERC20 tokens, however it can fail for decimal other than 18, such as WBTC (one of the key collateral token) which has 8 decimal on Ethereum Mainnet.

There are two fold issue with this :

  1. liquidate() function uses getTokenAmountFromUsd() wherein it returns the token amount in 18 decimals instead of the tokens decimal (8 for the case-in-point). as a result protocols liquidate() ends up consider 10,000,000,000 times more collateral than it is supposed to, essentially it will revert in most cases (as the chances of any user having that much collateral are very rare).

function getTokenAmountFromUsd(address token, uint256 usdAmountInWei) public view returns (uint256) {
// price of ETH (token)
// $/ETH ETH ??
// $2000 / ETH. $1000 = 0.5 ETH
AggregatorV3Interface priceFeed = AggregatorV3Interface(s_priceFeeds[token]);
(, int256 price,,,) = priceFeed.staleCheckLatestRoundData();
// ($10e18 * 1e18) / ($2000e8 * 1e10)
//@note - here the precision of usdAmountInWei is return (which is always fixed to : 18)
return (usdAmountInWei * PRECISION) / (uint256(price) * ADDITIONAL_FEED_PRECISION);
}

  • Health factor calculations in _revertIfHealthFactorIsBroken() accounts for all of the users collateral token values in USD, then sums it up in for loop. At that time getUsdValue() is used to fetch the USD values.

    • Problem here is that it returns these value in tokens decimal instead of 18 decimal, As a result total value summation will be incorrect.

    • Lets consider the case wherein the user has put in 1 token of WETH(18 Decimals) and 1 token of WBTC(8 Decimals) as collateral. Lets Also assume WETH is at 2000 USD and 1 WBTC is at 25000 USD.

    • So, ideally its total collateral should be worth 27000 USD and ideally getAccountCollateralValue() should return 27000e18.

function getAccountCollateralValue(address user) public view returns (uint256 totalCollateralValueInUsd) {
// loop through each collateral token, get the amount they have deposited, and map it to
// the price, to get the USD value
//@note - getUsdValue() returns the price of each token in its own decimal, summing over different decimal token will create issues.
for (uint256 i = 0; i < s_collateralTokens.length; i++) {
address token = s_collateralTokens[i];
uint256 amount = s_collateralDeposited[user][token];
totalCollateralValueInUsd += getUsdValue(token, amount);
}
return totalCollateralValueInUsd;
}

  • But, what will happen is that getUsdValue() will return 2000e18 for WETH as it should and in case of WBTC it will return 25000e8 (instead of 25000e18).

function getUsdValue(address token, uint256 amount) public view returns (uint256) {
AggregatorV3Interface priceFeed = AggregatorV3Interface(s_priceFeeds[token]);
(, int256 price,,,) = priceFeed.staleCheckLatestRoundData();
// 1 ETH = $1000
// The returned value from CL will be 1000 * 1e8
//@note - this will return ansewer in "amount" variables precision, instead of 1e18
return ((uint256(price) * ADDITIONAL_FEED_PRECISION) * amount) / PRECISION;
}

  • As a result getAccountCollateralValue() will return 2000000002500000000000 (~2000e18) implying collateral is worth ~2000 USD instead of 27000 USD.

  • _revertIfHealthFactorIsBroken() end up reverting where it should not, essentially breaking the protocol for many users.

Impact

Since _revertIfHealthFactorIsBroken() and liquidate() will be broken due to this, it will lead to manyb users being DOS of the many important functionality lead to their funds being stuck in protocol forever.

Tools Used

Manual Review

Recommendations

  • Implement proper precision calculation considering the decimals of token under calculation for liquidate() functions getTokenAmountFromUsd() so that it returns the token amount in tokens native decimals

function getTokenAmountFromUsd(address token, uint256 usdAmountInWei) public view returns (uint256) {
// price of ETH (token)
// $/ETH ETH ??
// $2000 / ETH. $1000 = 0.5 ETH
AggregatorV3Interface priceFeed = AggregatorV3Interface(s_priceFeeds[token]);
(, int256 price,,,) = priceFeed.staleCheckLatestRoundData();
// ($10e18 * 1e18) / ($2000e8 * 1e10)
---- return (usdAmountInWei * PRECISION) / (uint256(price) * ADDITIONAL_FEED_PRECISION);
++++ return (usdAmountInWei * 10**(IERC20(token).decimals()) / (uint256(price) * ADDITIONAL_FEED_PRECISION);
​
}
​

  • In similar way for the _revertIfHealthFactorIsBroken() function implement precision calculation such that all the USD values return by getUsdValue() are in 18 decimals, which will be summed up accurately in getAccountCollateralValue() leading to proper calculation.

function getUsdValue(address token, uint256 amount) public view returns (uint256) {
AggregatorV3Interface priceFeed = AggregatorV3Interface(s_priceFeeds[token]);
(, int256 price,,,) = priceFeed.staleCheckLatestRoundData();
// 1 ETH = $1000
// The returned value from CL will be 1000 * 1e8
---- return ((uint256(price) * ADDITIONAL_FEED_PRECISION) * amount) / PRECISION;
++++ return ((uint256(price) * (10**18 - 10**(IERC20(token).decimals() ) * amount) / PRECISION;
}
​

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.