DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Rounding-up of user's `cRatio` causes loss for the protocol

t0x1c

Summary

At multiple places in the code, user's collateral ratio has been calculated in a manner which causes loss of precision (rounding-up) due to division before multiplication. This causes potential loss for the DittoETH protocol, among other problems.

Root Cause

Use of the following piece of code causes rounding-up:

  • Style 1

uint256 cRatio = short.getCollateralRatioSpotPrice(LibOracle.getSavedOrSpotOraclePrice(asset));

  • Style 2

uint256 oraclePrice = LibOracle.getOraclePrice(asset); // or uint256 oraclePrice = LibOracle.getSavedOrSpotOraclePrice(asset); // or uint256 oraclePrice = LibOracle.getPrice(asset);
...
...
...
uint256 cRatio = short.getCollateralRatioSpotPrice(oraclePrice);

Vulnerability Details

Let's break the issue down into 4 smaller parts:

PART 1:

Let us first look inside getOraclePrice():

File: contracts/libraries/LibOracle.sol
​
20 function getOraclePrice(address asset) internal view returns (uint256) {
21 AppStorage storage s = appStorage();
22 AggregatorV3Interface baseOracle = AggregatorV3Interface(s.baseOracle);
23 uint256 protocolPrice = getPrice(asset);
24 // prettier-ignore
25 (
26 uint80 baseRoundID,
27 int256 basePrice,
28 /*uint256 baseStartedAt*/
29 ,
30 uint256 baseTimeStamp,
31 /*uint80 baseAnsweredInRound*/
32 ) = baseOracle.latestRoundData();
33
34 AggregatorV3Interface oracle = AggregatorV3Interface(s.asset[asset].oracle);
35 if (address(oracle) == address(0)) revert Errors.InvalidAsset();
36
37 @> if (oracle == baseOracle) {
38 //@dev multiply base oracle by 10**10 to give it 18 decimals of precision
39 uint256 basePriceInEth = basePrice > 0
40 @> ? uint256(basePrice * Constants.BASE_ORACLE_DECIMALS).inv()
41 : 0;
42 basePriceInEth = baseOracleCircuitBreaker(
43 protocolPrice, baseRoundID, basePrice, baseTimeStamp, basePriceInEth
44 );
45 @> return basePriceInEth;
46 } else {
47 // prettier-ignore
48 (
49 uint80 roundID,
50 int256 price,
51 /*uint256 startedAt*/
52 ,
53 uint256 timeStamp,
54 /*uint80 answeredInRound*/
55 ) = oracle.latestRoundData();
56 @> uint256 priceInEth = uint256(price).div(uint256(basePrice));
57 oracleCircuitBreaker(
58 roundID, baseRoundID, price, basePrice, timeStamp, baseTimeStamp
59 );
60 @> return priceInEth;
61 }
62 }

Based on whether the oracle is baseOracle or not, the function returns either basePriceEth or priceInEth.

  • basePriceEth can be uint256(basePrice * Constants.BASE_ORACLE_DECIMALS).inv() which is basically 1e36 / (basePrice * Constants.BASE_ORACLE_DECIMALS) or simply written, of the form oracleN / oracleD where oracleN is the numerator with value 1e36 (as defined here) and oracleD is the denominator.

  • priceInEth is given as uint256 priceInEth = uint256(price).div(uint256(basePrice)) which again is of the form oracleN / oracleD.


PART 2:

getSavedOrSpotOraclePrice() too internally calls the above getOraclePrice() function, if it has been equal to or more than 15 minutes since the last time LibOrders.getOffsetTime() was set:

File: contracts/libraries/LibOracle.sol
​
153 function getSavedOrSpotOraclePrice(address asset) internal view returns (uint256) {
154 if (LibOrders.getOffsetTime() - getTime(asset) < 15 minutes) {
155 return getPrice(asset);
156 } else {
157 @> return getOraclePrice(asset);
158 }
159 }

PART 3:

getCollateralRatioSpotPrice() calculates cRatio as:

File: contracts/libraries/LibShortRecord.sol
​
30 function getCollateralRatioSpotPrice(
31 STypes.ShortRecord memory short,
32 uint256 oraclePrice
33 ) internal pure returns (uint256 cRatio) {
34 @> return short.collateral.div(short.ercDebt.mul(oraclePrice));
35 }

PART 4 (FINAL PART):

There are multiple places in the code (mentioned below under Impacts section) which compare the user's cRatio to initialCR or LibAsset.primaryLiquidationCR(_asset) in the following manner:

if (short.getCollateralRatioSpotPrice(LibOracle.getSavedOrSpotOraclePrice(asset)) < LibAsset.primaryLiquidationCR(asset))

Calling short.getCollateralRatioSpotPrice(LibOracle.getSavedOrSpotOraclePrice(asset)) means the value returned from it would be:

// @audit-issue : Potential precision loss. Division before multiplication should not be done.
shortCollateral / (shortErcDebt * (oracleN / oracleD)) // return short.collateral.div(short.ercDebt.mul(oraclePrice));

which has the potential for precision loss (rounding-up) due to division before multiplication. The correct style ought to be:

+ @> (shortCollateral * oracleD) / (shortErcDebt * oracleN)

Impacts

  • Impact #1: User gets more dittoYieldShares than expected due to code logic:

File: contracts/facets/YieldFacet.sol
​
76 function _distributeYield(address asset)
77 private
78 onlyValidAsset(asset)
79 returns (uint88 yield, uint256 dittoYieldShares)
80 {
81 uint256 vault = s.asset[asset].vault;
82 // Last updated zethYieldRate for this vault
83 uint80 zethYieldRate = s.vault[vault].zethYieldRate;
84 // Protocol time
85 uint256 timestamp = LibOrders.getOffsetTimeHours();
86 // Last saved oracle price
87 @> uint256 oraclePrice = LibOracle.getPrice(asset);
88 // CR of shortRecord collateralized at initialMargin for this asset
89 uint256 initialCR = LibAsset.initialMargin(asset) + 1 ether;
90 // Retrieve first non-HEAD short
91 uint8 id = s.shortRecords[asset][msg.sender][Constants.HEAD].nextId;
92 // Loop through all shorter's shorts of this asset
93 while (true) {
94 // One short of one shorter in this market
95 STypes.ShortRecord storage short = s.shortRecords[asset][msg.sender][id];
96 // To prevent flash loans or loans where they want to deposit to claim yield immediately
97 bool isNotRecentlyModified =
98 timestamp - short.updatedAt > Constants.YIELD_DELAY_HOURS;
99 // Check for cancelled short
100 if (short.status != SR.Cancelled && isNotRecentlyModified) {
101 uint88 shortYield =
102 short.collateral.mulU88(zethYieldRate - short.zethYieldRate);
103 // Yield earned by this short
104 yield += shortYield;
105 // Update zethYieldRate for this short
106 short.zethYieldRate = zethYieldRate;
107 // Calculate CR to modify ditto rewards
108 @> uint256 cRatio = short.getCollateralRatioSpotPrice(oraclePrice);
109 @> if (cRatio <= initialCR) {
110 @> dittoYieldShares += shortYield;
111 } else {
112 // Reduce amount of yield credited for ditto rewards proportional to CR
113 dittoYieldShares += shortYield.mul(initialCR).div(cRatio);
114 }
115 }
116 // Move to next short unless this is the last one
117 if (short.nextId > Constants.HEAD) {
118 id = short.nextId;
119 } else {
120 break;
121 }
122 }
123 }

This rounding-up can lead to user's cRatio to be considered as >initialCR even when it's slightly lower. This results in greater dittoYieldShares being calculated.

File: contracts/facets/MarginCallPrimaryFacet.sol
​
43 function flagShort(address asset, address shorter, uint8 id, uint16 flaggerHint)
44 external
45 isNotFrozen(asset)
46 nonReentrant
47 onlyValidShortRecord(asset, shorter, id)
48 {
49 if (msg.sender == shorter) revert Errors.CannotFlagSelf();
50 STypes.ShortRecord storage short = s.shortRecords[asset][shorter][id];
51 short.updateErcDebt(asset);
52
53 if (
54 @> short.getCollateralRatioSpotPrice(LibOracle.getSavedOrSpotOraclePrice(asset))
55 @> >= LibAsset.primaryLiquidationCR(asset) // @audit-issue : this will evaluate to `true` due to rounding-up and the short will not be eligible for flagging
56 ) {
57 revert Errors.SufficientCollateral();
58 }
59
60 uint256 adjustedTimestamp = LibOrders.getOffsetTimeHours();
61
62 // check if already flagged
63 if (short.flaggerId != 0) {
64 uint256 timeDiff = adjustedTimestamp - short.updatedAt;
65 uint256 resetLiquidationTime = LibAsset.resetLiquidationTime(asset);
66
67 if (timeDiff <= resetLiquidationTime) {
68 revert Errors.MarginCallAlreadyFlagged();
69 }
70 }
71
72 short.setFlagger(cusd, flaggerHint);
73 emit Events.FlagShort(asset, shorter, id, msg.sender, adjustedTimestamp);
74 }

File: contracts/facets/MarginCallSecondaryFacet.sol
​
38 function liquidateSecondary(
39 address asset,
40 MTypes.BatchMC[] memory batches,
41 uint88 liquidateAmount,
42 bool isWallet
43 ) external onlyValidAsset(asset) isNotFrozen(asset) nonReentrant {
44 STypes.AssetUser storage AssetUser = s.assetUser[asset][msg.sender];
45 MTypes.MarginCallSecondary memory m;
46 uint256 minimumCR = LibAsset.minimumCR(asset);
47 uint256 oraclePrice = LibOracle.getSavedOrSpotOraclePrice(asset);
48 uint256 secondaryLiquidationCR = LibAsset.secondaryLiquidationCR(asset);
49
50 uint88 liquidatorCollateral;
51 uint88 liquidateAmountLeft = liquidateAmount;
52 for (uint256 i; i < batches.length;) {
53 @> m = _setMarginCallStruct(
54 asset, batches[i].shorter, batches[i].shortId, minimumCR, oraclePrice
55 );
56
​
......
......
......
​
​
129 function _setMarginCallStruct(
130 address asset,
131 address shorter,
132 uint8 id,
133 uint256 minimumCR,
134 uint256 oraclePrice
135 ) private returns (MTypes.MarginCallSecondary memory) {
136 LibShortRecord.updateErcDebt(asset, shorter, id);
137
138 MTypes.MarginCallSecondary memory m;
139 m.asset = asset;
140 m.short = s.shortRecords[asset][shorter][id];
141 m.vault = s.asset[asset].vault;
142 m.shorter = shorter;
143 m.minimumCR = minimumCR;
144 @> m.cRatio = m.short.getCollateralRatioSpotPrice(oraclePrice);
145 return m;
146 }
​

File: contracts/facets/ShortRecordFacet.sol
​
117 function combineShorts(address asset, uint8[] memory ids)
118 external
119 isNotFrozen(asset)
120 nonReentrant
121 onlyValidShortRecord(asset, msg.sender, ids[0])
122 {
123 if (ids.length < 2) revert Errors.InsufficientNumberOfShorts();
124 // First short in the array
125 STypes.ShortRecord storage firstShort = s.shortRecords[asset][msg.sender][ids[0]];
......
......
......
​
174
175 // Merge all short records into the short at position id[0]
176 firstShort.merge(ercDebt, ercDebtSocialized, collateral, yield, c.shortUpdatedAt);
177
178 // If at least one short was flagged, ensure resulting c-ratio > primaryLiquidationCR
179 if (c.shortFlagExists) {
180 if (
181 @> firstShort.getCollateralRatioSpotPrice(
182 @> LibOracle.getSavedOrSpotOraclePrice(_asset)
183 @> ) < LibAsset.primaryLiquidationCR(_asset)
184 ) revert Errors.InsufficientCollateral();
185 // Resulting combined short has sufficient c-ratio to remove flag
186 firstShort.resetFlag();
187 }
188 emit Events.CombineShorts(asset, msg.sender, ids);
189 }

NOTE:


While the operation done in this piece of code is a bit different from the above analysis, I am clubbing it with this bug report as the underlying issue is the same (and the resolution would be similar): Multiplication and division operations should not be done directly on top of fetched oracle price, without paying attention to new order of evaluation:

File: contracts/libraries/LibOrders.sol
​
812 function _updateOracleAndStartingShort(address asset, uint16[] memory shortHintArray)
813 private
814 {
815 AppStorage storage s = appStorage();
815 uint256 oraclePrice = LibOracle.getOraclePrice(asset);
......
......
......
​
845 //@dev: force hint to be within 1% of oracleprice
846 bool startingShortWithinOracleRange = shortPrice
847 @> <= oraclePrice.mul(1.01 ether) // @audit-issue : division before multiplication
848 && s.shorts[asset][prevId].price >= oraclePrice;
......
......
......
​
866 }

The effective calculation being done above is:

(oracleN / oracleD) * (1.01 ether) // division before multiplication

Which should have been:

(oracleN * 1.01 ether) / oracle

Similar multiplication or division operations have been done on price at various places throughout the code, which can be clubbed under this root cause itself.

PoC

Have attempted to keep all values in close proximity to the ones present in forked mainnet tests.

Let's assume some values for numerator & denominator and other variables:

uint256 private short_collateral = 100361729669569000000; // ~ 100 ether
uint256 private short_ercDebt = 100000000000000000000000; // 100_000 ether
uint256 private price = 99995505; // oracleN
uint256 private basePrice = 199270190598; // oracleD
uint256 private primaryLiquidationCR = 2000000000000000000; // 2 ether (as on forked mainnet)
​
// For this example, we assume that oracle != baseOracle, so that the below calculation would be done by the protocol
So calculated priceInEth = price.div(basePrice) = 501808648347845 // ~ 0.0005 ether

Let's calculate for the scenario of flagShort() where the code logic says:

53 if (
54 @> short.getCollateralRatioSpotPrice(LibOracle.getSavedOrSpotOraclePrice(asset))
55 @> >= LibAsset.primaryLiquidationCR(asset) // @audit-issue : this will evaluate to `true`, then revert, due to rounding-up and the short will incorrectly escape flagging
56 ) {
57 revert Errors.SufficientCollateral();
58 }

Create a file named test/IncorrectCRatioCheck.t.sol and paste the following code in it. Some mock functions are included here which mirror protocol's calculation style:

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.21;
​
import {U256} from "contracts/libraries/PRBMathHelper.sol";
import {OBFixture} from "test/utils/OBFixture.sol";
import {console} from "contracts/libraries/console.sol";
​
contract IncorrectCRatioCheck is OBFixture {
using U256 for uint256;
​
uint256 private short_collateral = 85307470219133700000; // ~ 85.3 ether
uint256 private short_ercDebt = 100000000000000000000000; // 100_000 ether
uint256 private price = 99995505; // oracleN
uint256 private basePrice = 199270190598; // (as on forked mainnet) // oracleD
uint256 private primaryLiquidationCR = 1700000000000000000; // 1.7 ether (as on forked mainnet)
​
function _getSavedOrSpotOraclePrice() internal view returns (uint256) {
uint256 priceInEth = price.div(basePrice);
return priceInEth; // will return 501808648347845 =~ 0.0005 ether // (as on forked mainnet)
}
​
function getCollateralRatioSpotPrice_IncorrectStyle_As_In_Existing_DittoProtocol(
uint256 oraclePrice
) internal view returns (uint256) {
return short_collateral.div(short_ercDebt.mul(oraclePrice));
}
​
function getCollateralRatioSpotPrice_CorrectStyle(uint256 oracleN, uint256 oracleD)
internal
view
returns (uint256)
{
return (short_collateral.mul(oracleD)).div(short_ercDebt.mul(oracleN));
}
​
/* solhint-disable no-console */
function test_GetCollateralRatioSpotPrice_IncorrectStyle_As_In_Existing_DittoProtocol(
) public view {
uint256 cRatio =
getCollateralRatioSpotPrice_IncorrectStyle_As_In_Existing_DittoProtocol(
_getSavedOrSpotOraclePrice()
);
console.log("cRatio calculated (existing style) =", cRatio);
if (cRatio >= primaryLiquidationCR) {
console.log("Errors.SufficientCollateral; can not be flagged");
} else {
console.log("InsufficientCollateral; can be flagged");
}
}
​
/* solhint-disable no-console */
function test_GetCollateralRatioSpotPrice_CorrectStyle() public view {
uint256 cRatio = getCollateralRatioSpotPrice_CorrectStyle(price, basePrice);
console.log("cRatio calculated (correct style) =", cRatio);
if (cRatio >= primaryLiquidationCR) {
console.log("Errors.SufficientCollateral; can not be flagged");
} else {
console.log("InsufficientCollateral; can be flagged");
}
}
}

  • First, let's see the output as per protocol's calculation. Run forge test --mt test_GetCollateralRatioSpotPrice_IncorrectStyle_As_In_Existing_DittoProtocol -vv:

Logs:
cRatio calculated (existing style) = 1700000000000000996
Errors.SufficientCollateral; can not be flagged

So the short can not be flagged as cRatio > primaryLiquidationCR of 1700000000000000000.



  • Now, let's see the output as per the correct calculation. Run forge test --mt test_GetCollateralRatioSpotPrice_CorrectStyle -vv:

Logs:
cRatio calculated (correct style) = 1699999999999899995
InsufficientCollateral; can be flagged

Short's cRatio is actually below primaryLiquidationCR. Should have been flagged ideally.


Tools Used

Manual review, forge test.

Recommendations

These steps need to be taken to fix the issue. Developer may have to make some additional changes since .mul, .div, etc are being used from the PRBMathHelper.sol library. Following is the general workflow required:

  • Create additional functions to fetch oracle parameters instead of price: Create copies of getOraclePrice() and getSavedOrSpotOraclePrice(), but these ones return oracleN & oracleD instead of the calculated price. Let's assume the new names to be getOraclePriceParams() and getSavedOrSpotOraclePriceParams().

  • Create a new function to calculate cRatio which will be used in place of the above occurences of getCollateralRatioSpotPrice():

function getCollateralRatioSpotPriceFromOracleParams(
STypes.ShortRecord memory short,
uint256 oracleN,
uint256 oracleD
) internal pure returns (uint256 cRatio) {
return (short.collateral.mul(oracleD)).div(short.ercDebt.mul(oracleN));
}

  • For fixing the last issue of oraclePrice.mul(1.01 ether) on L847, first call getOraclePriceParams() to get oracleN & oracleD and then:

845 //@dev: force hint to be within 1% of oracleprice
846 bool startingShortWithinOracleRange = shortPrice
- 847 <= oraclePrice.mul(1.01 ether)
+ 847 <= (oracleN.mul(1.01 ether)).div(oracleD)
848 && s.shorts[asset][prevId].price >= oraclePrice;
Updates

Lead Judging Commences

0xnevi Lead Judge
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-234

t0x1c Submitter
almost 2 years ago
0xnevi Lead Judge
almost 2 years ago
T1MOH Auditor
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-234

Support

FAQs

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