DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Users can lose yield in `disburseCollateral()` and `_distributeYield()` due to incorrect calculations

t0x1c

Summary

  • disburseCollateral() distributes yield to the user if isNotRecentlyModified is true i.e short has not been updated in the last 1 hour (Constants.YIELD_DELAY_HOURS).

  • _distributeYield() distributes yield to the user if isNotRecentlyModified is true i.e short has not been updated in the last 1 hour (Constants.YIELD_DELAY_HOURS).


Due to the bug in the code logic where it uses LibOrders.getOffsetTimeHours() for checking timestamp, users will lose their rightfully deserved yield.

I have clubbed both these issues into this single bug report as they both involve comparison with Constants.YIELD_DELAY_HOURS to distribute yield: bool isNotRecentlyModified = LibOrders.getOffsetTimeHours() - updatedAt > Constants.YIELD_DELAY_HOURS;

Vulnerability Details & Impact

In disburseCollateral(), variable isNotRecentlyModified is calculated as:

File: contracts/libraries/LibShortRecord.sol
​
349 /*
350 @dev If somebody exits a short, gets margin called, decreases their collateral before YIELD_DELAY_HOURS duration is up,
351 they lose their yield to the TAPP
352 */
353 @> bool isNotRecentlyModified =
354 @> LibOrders.getOffsetTimeHours() - updatedAt > Constants.YIELD_DELAY_HOURS;
355 if (isNotRecentlyModified) {
356 s.vaultUser[vault][shorter].ethEscrowed += yield;
357 } else {
358 s.vaultUser[vault][address(this)].ethEscrowed += yield;
359 }

In _distributeYield(), similar pattern:

File: contracts/facets/YieldFacet.sol
​
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;

The function LibOrders.getOffsetTimeHours() returns:

File: contracts/libraries/LibOrders.sol
​
30 function getOffsetTimeHours() internal view returns (uint24 timeInHours) {
31 @> return uint24(getOffsetTime() / 1 hours);
32 }

Any timestamp is rounded-down to the hour. So 1 hour 30 minutes would be considered as 1 hour and hence not greater than Constants.YIELD_DELAY_HOURS which is equal to 1 hour in the protocol currently.

Important to note that even the updatedAt value of an order is being set using LibOrders.getOffsetTimeHours() instead of using an exact timestamp, as can be seen here.

Let's see the problem this poses with an example:

Assume that the "starting time" or contract deployment time is at midnight i.e 00:00 .
Suppose short gets updated at timestamp: 5:05 . Protocol will store this in rounded-down form as `updatedAt` = 5:00
​
Suppose the protocol allows an action on this short after `X` hours. Here, X = 1.
So the user should ideally be able to perform the action after 6:05.
​
However, when he tries, at 6:06, his current time stamp is calculated (rounded-down) as 6:00. The `timeDiff` = 6:00 - 5:00 = 1 hour.
This is less than equal to X (Constants.YIELD_DELAY_HOURS in this case) i.e `timeDiff <= X`, so he won't be allowed to perform the action.
​
The user will have to wait another 54 minutes till 7:00 so that timeDiff is 2 hours.

isNotRecentlyModified remains false even if user has crossed the 1 hour mark, but is still within the 2 hour mark.

Impact

In the above scenario, user will lose the yield they rightfully deserved.

PoC-1

To test disburseCollateral() (which is internally called when external user calls decreaseCollateral()), paste the following code inside test/Yield.t.sol and run it via forge test --mt test_disburseCollateralFailsEvenAfter1Hour30Minutes -vv.

function test_disburseCollateralFailsEvenAfter1Hour30Minutes() public {
setUpShortAndCheckInitialEscrowed();
​
vm.prank(owner);
diamond.setTithe(vault, 0);
generateYield(10 ether);
skip(90 minutes);
vm.prank(sender);
decreaseCollateral(Constants.SHORT_STARTING_ID, 1 wei);
​
bool userReceivedCollateral =
diamond.getVaultUserStruct(vault, sender).ethEscrowed > (1000 ether + 1 wei);
bool nothingCreditedToTAPP =
diamond.getVaultUserStruct(vault, tapp).ethEscrowed == 0;
​
assertTrue(
userReceivedCollateral && nothingCreditedToTAPP,
"User did not receive yield & was taken by TAPP"
);
}

Output:

Logs:
Error: User did not receive yield & was taken by TAPP
Error: Assertion Failed

The protocol should have credited yield to the user after 90 minutes (1 hour 30 minutes), but did not and hence the test failed.

One can change the skip value to 120 minutes or 2 hours in the above test and see that it would pass. Basically the user is having to wait an extra hour (which he would have no way of knowing in advance), else he loses his yield to TAPP.

PoC-2

To test _distributeYield() (which is internally called when external user calls distributeYield()), paste the following code inside test/Yield.t.sol and run it via forge test --mt test_distributeYieldFailsEvenAfter1Hour59Minutes -vv.

function test_distributeYieldFailsEvenAfter1Hour59Minutes() public {
setUpShortAndCheckInitialEscrowed();
vm.startPrank(sender);
generateYield(1 ether);
​
address[] memory assets = new address[](1);
assets[0] = asset;
​
skip(1 hours + 59 minutes);
diamond.distributeYield(assets); // should pass, but incorrectly reverts
}

Output:

Running 1 test for test/Yield.t.sol:YieldTest
[FAIL. Reason: NoYield()] test_distributeYieldFailsEvenAfter1Hour59Minutes() (gas: 994959)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 154.83ms

The test should have passed as the user is past the 1 hour mark, but it reverts & fails. One can change the skip value to 2 hours in the above test and see that it would pass.

Tools Used

Manual review, foundry.

Recommendations

Instead of doing these calculations in hours, they should be done in seconds to avoid rounding error.

Updates

Lead Judging Commences

0xnevi Lead Judge
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Other
t0x1c Submitter
almost 2 years ago
0xnevi Lead Judge
almost 2 years ago
t0x1c Submitter
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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