Summary
Both the price and the amount for calculating the ethAmount are supposed to be denominated in ETH.
However, within the ExitShortFacet#exitShort() above, both the price and the buyBackAmount denominated in ERC (cUSD) would be used for calculating the ethAmount.
This lead to miscalculation of the ethAmount in the ExitShortFacet#exitShort().
Vulnerability Details
Within the ExitShortFacet#exitShort(), the ethAmount would be calculated based on the price of debt and the buyBackAmount of debt like this:
https://github.com/Cyfrin/2023-09-ditto/blob/main/contracts/facets/ExitShortFacet.sol#L183
* @notice Exits a short by placing bid on market
* @dev allows for partial exit via buyBackAmount
*
* @param asset The market that will be impacted
* @param id Id of short
* @param buyBackAmount Erc amount to buy back
* @param price Price at which shorter wants to place bid
* @param shortHintArray Array of hintId for the id to start matching against shorts since you can't match a short < oracle price
*
*/
function exitShort(
address asset,
uint8 id,
uint88 buyBackAmount,
uint80 price,
uint16[] memory shortHintArray
)
external
isNotFrozen(asset)
nonReentrant
onlyValidShortRecord(asset, msg.sender, id)
{
MTypes.ExitShort memory e;
e.asset = asset;
LibOrders.updateOracleAndStartingShortViaTimeBidOnly(
e.asset, OF.FifteenMinutes, shortHintArray
);
STypes.Asset storage Asset = s.asset[e.asset];
STypes.ShortRecord storage short = s.shortRecords[e.asset][msg.sender][id];
short.updateErcDebt(e.asset);
e.beforeExitCR = short.getCollateralRatio(e.asset);
e.ercDebt = short.ercDebt;
e.collateral = short.collateral;
if (buyBackAmount == 0 || buyBackAmount > e.ercDebt) {
revert Errors.InvalidBuyback();
}
if (e.ercDebt > buyBackAmount) {
uint256 leftoverAmt = (e.ercDebt - buyBackAmount).mul(price);
if (leftoverAmt < LibAsset.minBidEth(e.asset)) {
revert Errors.CannotLeaveDustAmount();
}
}
{
uint256 ethAmount = price.mul(buyBackAmount);
if (ethAmount > e.collateral) revert Errors.InsufficientCollateral();
}
...
According to the NatSpect of the ExitShortFacet#exitShort() above, the price and the buyBackAmount would be described like this:
@param buyBackAmount Erc amount to buy back
@param price Price at which shorter wants to place bid
Based on the NatSpect of the ExitShortFacet#exitShort() above, both the price and the buyBackAmount are supposed to be denominated in ERC (cUSD).
On the other hand, both the price and the amount for calculating the ethAmount are supposed to be denominated in ETH.
However, within the ExitShortFacet#exitShort() above, both the price and the buyBackAmount denominated in ERC (cUSD) would be used for calculating the ethAmount.
This lead to miscalculation of the ethAmount in the ExitShortFacet#exitShort().
Impact
This lead to miscalculation of the ethAmount in the ExitShortFacet#exitShort().
Tools Used
Recommendations
Within the ExitShortFacet#exitShort(), consider replacing both the price (in ERC/cUSD) and the buyBackAmount (in ERC/cUSD) with the the price in ETH and the buyBackAmount in ETH like this:
function exitShort(
address asset,
uint8 id,
uint88 buyBackAmount,
uint80 price,
uint16[] memory shortHintArray
)
external
isNotFrozen(asset)
nonReentrant
onlyValidShortRecord(asset, msg.sender, id)
{
MTypes.ExitShort memory e;
e.asset = asset;
LibOrders.updateOracleAndStartingShortViaTimeBidOnly(
e.asset, OF.FifteenMinutes, shortHintArray
);
STypes.Asset storage Asset = s.asset[e.asset];
STypes.ShortRecord storage short = s.shortRecords[e.asset][msg.sender][id];
short.updateErcDebt(e.asset);
e.beforeExitCR = short.getCollateralRatio(e.asset);
e.ercDebt = short.ercDebt;
e.collateral = short.collateral;
if (buyBackAmount == 0 || buyBackAmount > e.ercDebt) {
revert Errors.InvalidBuyback();
}
if (e.ercDebt > buyBackAmount) {
uint256 leftoverAmt = (e.ercDebt - buyBackAmount).mul(price);
if (leftoverAmt < LibAsset.minBidEth(e.asset)) {
revert Errors.CannotLeaveDustAmount();
}
}
{
+ uint256 ethAmount = price_in_ETH.mul(buyBackAmount_in_ETH);
- uint256 ethAmount = price.mul(buyBackAmount);
if (ethAmount > e.collateral) revert Errors.InsufficientCollateral();
}
