Race condition in exitShortFacet::exitShortErcEscrowed could refunding the full collateral

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/ExitShortFacet.sol#L87-L130

Vulnerability details

Impact

When the original exitShortErcEscrowed function from User A resumes, it will use the outdated values for ercDebt and collateral, potentially leading to inconsistencies or unexpected behavior.

For example, the function may incorrectly refund the full collateral amount if the ercDebt is now less than the buybackAmount, even though the ercDebt was modified by User B's transaction.

Proof of Concept

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/ExitShortFacet.sol#L87C5-L130C1

User A calls the exitShortErcEscrowed function with a valid buybackAmount. The function first updates the ercEscrowed value, reducing it by the buybackAmount. Before the function can update the ercDebt and collateral values, another transaction, e.g., from User B, modifies the ercDebt or collateral values for the same asset and user. When the original exitShortErcEscrowed function resumes, it will use the outdated values, potentially leading to inconsistencies or unexpected behavior.

OR consider this scenario:

User A calls the exitShortErcEscrowed function with the following parameters:

asset: 0x1234... id: 1 buybackAmount: 100 shortOrderId: 123

The function first updates the ercEscrowed value for the user's asset:

s.assetUser[asset][msg.sender].ercEscrowed is reduced by 100.

Before the function can update the ercDebt and collateral values, a malicious user, User B, calls another function that modifies the ercDebt or collateral values for the same asset and user.

For example, User B calls the updateShortPosition function with the following parameters:

asset: 0x1234... id: 1 newCollateral: 200 newErcDebt: 150 This updates the collateral and ercDebt values for the same asset and user.

When the original exitShortErcEscrowed function from User A resumes, it will use the outdated values for ercDebt and collateral, potentially leading to inconsistencies or unexpected behavior.

The function may incorrectly refund the full collateral amount if the ercDebt is now less than the buybackAmount, even though the ercDebt was modified by User B's transaction.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this race condition, the function should perform all the state updates (ercEscrowed, ercDebt, and collateral) within a single transaction or use appropriate locking mechanisms to ensure atomicity and prevent race conditions.

Assessed type

Timing

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Readme: Issues related to front-running: can front-run someone's order, liquidation, the chainlink/uniswap oracle update.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Out of scope