DittoETH

Summary

Decreasing collateral after increasing it causes permanent loss of yield for the user if done within a timespan of Constants.YIELD_DELAY_HOURS with cRatio at a healthy level.

Similar loss of yield is observed if user tries exiting a short using exitShort..() functions, instead of decreasing the collateral.

Vulnerability Details & Impacts

Developer has provided the following comment inside disburseCollateral():

However, the unintended side effect of this is visible in the following flow of events:

• Step 1: User (Shorter / Receiver) has a fully-filled short record and hence is eligible for a yield after certain duration of time. We skip this time-duration (1 hour in this case or Constants.YIELD_DELAY_HOURS). His cRatio >= LibAsset.primaryLiquidationCR(asset) is true at this moment.

• Step 2: User calls increaseCollateral() with any amount. The cRatio >= LibAsset.primaryLiquidationCR(asset) remains true, and this resets updatedAt via a call to short.resetFlag() and hence isNotRecentlyModified will evaluate to false in Line 353 above if checked within a duration of YIELD_DELAY_HOURS.

• Step 3: User calls decreaseCollateral() with any eligible amount before YIELD_DELAY_HOURS duration is up.

• Step 3 variation-1 : User calls exitShortWallet() for a full-exit before YIELD_DELAY_HOURS duration is up.

• Step 3 variation-2 : User calls exitShortErcEscrowed() for a full-exit before YIELD_DELAY_HOURS duration is up.

• Step 3 variation-3 : User calls exitShort() for an exit (partial or full) before YIELD_DELAY_HOURS duration is up.
  
  Result: Any yield he was eligible for, goes to TAPP and he receives nothing.
  
  

I also reference the docs which give the reason for such a logic:

disburseCollateral is an internal function that is called whenever collateral "leaves" a shortRecord. This is similar to distributeYield but pertains to a particular shortRecord and distributes yield only for the amount of collateral that is being decreased. This way unrealized yield that was not distributed is captured before permanent loss.

YIELD_DELAY_HOURS is also utilized here to prevent flash loan exploits that quickly make shorts and exit them. When this time condition is not satisfied, the distributed yield is sent to the TAPP instead of the shortRecord owner.

Also here:

Eligibility Criteria:

- Only applies to shortRecord.
- shortRecord created/modified more than YIELD_DELAY_HOURS ago.
    - Upon creation, shortRecord starts accruing yield, but it's only eligible to claim after waiting YIELD_DELAY_HOURS.

The intention of the protocol is to cancel the yield (transfer it to TAPP) if it has been accrued within the last hour. However, any yield rightfully accrued from previously present collateral (which could have been sitting for last 5 hours), can be claimed by the user. This is violated as shown in the following PoC.

PoC

Add the following tests inside test/Yield.t.sol. Also, uncomment Line 11 so that we can do some logging.

Let's run the "normal" scenario first, where the user has some accumulated yield and see the actual figures. These would be our expected values. Run forge test --mt test_normal_decreaseCollateral -vv to get the output:

The yield credited to the user here is final_ethEsrowed - initial_ethEsrowed - collateralWithdrawn = 1001300000000000000000 - 1000000000000000000000 - 1000000000000000000 = 300000000000000000 = 0.3 ether.


Now, run forge test --mt test_bug_incThenDecreaseCollateral -vv to get the output:

User receives no yield as his final ethEsrowed remains at initial_ethEsrowed + collateralWithdrawn - collateralAdded = 1000000000000000000000 + 1000000000000000000 - 1 = 1000999999999999999999 = 1000.999999999999999999 ether.

All the yield goes to TAPP: yield_deposited_in_tapp = 500000000000000000 - 200000000000000000 (from the first test) = 300000000000000000 = 0.3 ether --> which equals the yield user had received in the first test.

Tools Used

Manual review, forge.

Recommendations

If cRatio >= LibAsset.primaryLiquidationCR(asset) is already true when user calls increaseCollateral(), then we need a variable to track the yield (or collateral) which the user is so far eligible to receive. Doing this will protect the protocol from giving the user any yield accrued spontaneously from the newly added collateral (via flash-loan or otherwise) and at the same time protect the old accrued yield user rightfully should receive while calling decreaseCollateral().