DittoETH

Summary

SR.Cancelled short record can be overwritten to SR.FullyFilled and a malicious user can withdraw the collateral twice

Vulnerability Details

Users can combine their short records by calling combineShorts function, which delete all of the inputed short records and combine them to only one short record and this function make a call to deleteShortRecord function in deleteShortRecord library which removes it's id from the market if shortRecord.status != SR.PartialFill (by changing the prevId and nextId) and also mark the short record as cancelled whether it's state is PartialFill or FullyFilled on line 224

The problem is whenever you delete a short record you don't require its order to be fulfilled (means short order is still active in the order book and it may get fulfilled any time) and also you don't set its collateral, ercDebt and other values to 0 (they still the same value)

now lets say a combined short record which is canelled but it still have ercAmount left in its order (active in the order book), gets matched with bid and whenever a short order get matched you call BidOrdersFacet::matchlowestSell function and this code snippet will get invoked:

On line 280 you check to see if incomingBid ercAmount is greater than short order's ercAmount if it is you set the status to SR.FullyFilled and since the short order already has short record the fillShortRecord will be called and all of the parameters importantly status will be passed. this means if the short record is previously marked as SR.Cancelled via combining shorts, exiting short or liquidated it will get overwritten by SR.FullyFilled.

Now a malicious user who has combined his short record previously can combine the same short record again since he can pass onlyValidShortRecord modifier succesfully because it is not cancelled any more this means he will get double collateral

This vaunerability exist for combining shorts, exiting short and also liquidations

Impact

Imagaine an attacker creates a short order after that some amount of the order gets matched with a bid and short record will be created for that order, now attacker calls exitShortWallet or exitShortErcEscrowed function in ExitShortFacet contract by paying back 99% amount of ercDebt (this reduce the ercDebt, if he pay all of it on first call the ercDebt will not be reduced and delete function will be invoked directly) and again call exit functions this time by paying back all of the remaining ercDebt which invokes delete function and the short record will be marked as SR.Cancelled (Notice: when short record is deleted the shortRecordId will not be updated in short order's struct and it still the same deleted one).

He waits some time till the remaining of the order gets matched and the deleted short record's values will be updated and also the status will be overwritten from SR.Cancelled to SR.FullyFilled. now he can call exitShortWallet by passing the same short record's id and succesfully pass onlyValidShortRecord(asset, msg.sender, id) modifier, this way attacker will get double collateral plus their new added collateral for remaining of the order.

if collateral is supposed to be 1 ETH attacker will get 2 ETH by applying this attack (attacker only pay back ercDebt once and not double since it is reduced but collateral is not)

Attacker can repeat this attack till he drain all of the funds from the contract

Tools Used

Manual Review

Recommendations

In case of combiniing shorts it's better to only allow fulfilled short records be combined but for exiting short and liquidating it you need some functionalty to update the shortRecordId of orders that points to the short record that needs to be deleted (shortRecordId of that order need to be updated to 0 zero that way if it is not fulfilled new short record will be created for new collateral and ercDebt).