Open c4-bot-9 opened 6 months ago
raymondfam marked the issue as insufficient quality report
raymondfam marked the issue as primary issue
Inadequate/unstructured proof to support the intended code refactoring.
hansfriese marked the issue as unsatisfactory: Insufficient proof
Hi @hansfriese,
Appeal
Please have a second look at the following from the Proof of Concept
section:
While processing the
proposalInput
param, theproposeRedemption()
will load theshortOrder
from storage according to the attacker-inputtedshortOrderId
param (i.e.,p.shortOrderId
).Suppose that the attacker wants to leave their small
shortRecord
position to disincentivize liquidators from liquidating it (as well as disabling the redemption mechanism from redeeming it forethCollateral
). They can place their partially-filledshortRecord
that has the correspondingshortOrder.ercAmount
<minShortErc
to be redeemed via a Sybil account.To bypass the
minShortErc
threshold verification process from canceling their smallshortOrder
, the attacker must specify thep.shortOrderId
param to anothershortOrder
's id withercAmount
>=minShortErc
. The manipulatedp.shortOrderId
param can bypass the verification process because if theloaded shortOrder.ercAmount
>=minShortErc
, theproposeRedemption()
will not proceed to verify the validity of theshortOrder
.Hence, the attacker can target their
shortRecord
to be redeemed and leave its correspondingshortOrder
withercAmount
<minShortErc
to keep alive on the order book. Once their smallshortOrder
is matched again, it will leave theshortRecord
position less than theminShortErc
threshold, disincentivizing liquidators from liquidating it.Furthermore, the small
shortRecord
(i.e.,shortRecord.ercDebt
<minShortErc
) also disables the redemption mechanism from redeeming it forethCollateral
.
To demystify the vulnerability again, please follow the links along:
The shortOrder
is loaded from storage according to the attacker's p.shortOrderId
param.
The attacker can leave their target partially-filled shortOrder
(corresponding to the redeeming shortRecord
) with ercAmount
< minShortErc
alive on the order book by specifying the p.shortOrderId
param to another shortOrder
's id with ercAmount
>= minShortErc
(to bypass the if
statement).
The root cause is that the proposeRedemption()
will verify the loaded shortOrder
against the shortRecordId
and shorter
params only after the condition in Step 2 was met.
Since the attacker already bypassed the condition check in Step 2, Step 3 would not be executed.
Hence, if the attacker points the p.shortOrderId
param to another (fake) shortOrder
's id with ercAmount
>= minShortErc
, they can bypass the validity check of the loaded (fake) shortOrder
, preventing their targeted small shortOrder
(the real shortOrder
corresponding to the redeeming shortRecord
) from being canceled.
Subsequently, the attacker can leave their target small shortOrder
on the order book and when it is matched, its short position (shortRecord
) will be less than the minShortErc
threshold that will disincentivize liquidators from liquidating the position even if it is liquidable, leading to the protocol's bad debt. Moreover, the small shortRecord
(i.e., shortRecord.ercDebt
< minShortErc
) will also disable the redemption mechanism from redeeming it for ethCollateral
.
An attacker can leave small shortOrders
on the order book. As a result, large Bid
orders can run out of gas if the attacker places many small shortOrders
on the order book.
Liquidators are disincentivized from liquidating small shortRecords
because of their small amounts.
Small shortRecords
disable the redemption mechanism from redeeming them for ethCollateral
even if they have poor collateralization.
The snippet below presents how to fix the vulnerability by verifying the validity of the loaded shortOrder
(Step 1) against the shortRecordId
and shorter
params (Step 3) before processing the loaded shortOrder
(Step 2).
This way, an attacker cannot manipulate the p.shortOrderId
param to another (fake) shortOrder
's id.
function proposeRedemption(
address asset,
MTypes.ProposalInput[] calldata proposalInput,
uint88 redemptionAmount,
uint88 maxRedemptionFee
) external isNotFrozen(asset) nonReentrant {
...
for (uint8 i = 0; i < proposalInput.length; i++) {
p.shorter = proposalInput[i].shorter;
p.shortId = proposalInput[i].shortId;
p.shortOrderId = proposalInput[i].shortOrderId;
STypes.ShortRecord storage currentSR = s.shortRecords[p.asset][p.shorter][p.shortId];
...
STypes.Order storage shortOrder = s.shorts[asset][p.shortOrderId]; //@audit -- Step 1
+ if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder(); //@audit -- Process Step 3 before Step 2 to fix the vulnerability
if (currentSR.status == SR.PartialFill && shortOrder.ercAmount < minShortErc) { //@audit -- Step 2
- if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder(); //@audit -- Step 3
LibOrders.cancelShort(asset, p.shortOrderId);
}
...
}
...
}
This is valid, good find
ditto-eth (sponsor) confirmed
Nice finding.
Medium is appropriate as an attacker can bypass the minShortErc
validation.
hansfriese removed the grade
hansfriese marked the issue as satisfactory
hansfriese marked the issue as selected for report
Lines of code
https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L81 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L110-L114 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L38
Vulnerability details
Impact
The
BidOrdersFacet::bidMatchAlgo()
allows ashortOrder
to be partially matched and leave itsercAmount
*price
<minAskEth
due to theDUST_FACTOR
constant (as long as its correspondingshortRecord
is maintaining enoughercDebt
+shortOrder
'sercAmount
to keep the position >= theminShortErc
threshold).The redemption process enables redeemers to redeem their
ercEscrowed
forethCollateral
on targetshortRecords
. If ashortRecord
was partially filled, theRedemptionFacet::proposeRedemption()
must guarantee that the correspondingshortOrder
maintains theercAmount
>=minShortErc
. In other words, if theshortOrder
'sercAmount
is less than theminShortErc
threshold, theshortOrder
must be canceled from the order book. Otherwise, theshortRecord
position will be less than theminShortErc
threshold when the order is matched again. Subsequently, the smallshortRecord
(short
position) will not incentivize liquidators to liquidate it even if it is liquidable, leaving bad debt to the protocol.I discovered that the
proposeRedemption()
improperly verifies the proposer (redeemer)'s inputtedshortOrderId
param, allowing an attacker to specify theshortOrderId
param to anothershortOrder
's id that does not correspond to the target redeemingshortRecord
.The vulnerability can bypass the
minShortErc
threshold verification process on theshortOrder
corresponding to the processingshortRecord
, eventually allowing an attacker to leave a smallshortRecord
position that disincentivizes liquidators from liquidating the position. Furthermore, the smallshortRecord
also disables the redemption mechanism from redeeming it forethCollateral
.Proof of Concept
While processing the
proposalInput
param, theproposeRedemption()
will load theshortOrder
from storage according to the attacker-inputtedshortOrderId
param (i.e.,p.shortOrderId
).Suppose that the attacker wants to leave their small
shortRecord
position to disincentivize liquidators from liquidating it (as well as disabling the redemption mechanism from redeeming it forethCollateral
). They can place their partially-filledshortRecord
that has the correspondingshortOrder.ercAmount
<minShortErc
to be redeemed via a Sybil account.To bypass the
minShortErc
threshold verification process from canceling their smallshortOrder
, the attacker must specify thep.shortOrderId
param to anothershortOrder
's id withercAmount
>=minShortErc
. The manipulatedp.shortOrderId
param can bypass the verification process because if theloaded shortOrder.ercAmount
>=minShortErc
, theproposeRedemption()
will not proceed to verify the validity of theshortOrder
.Hence, the attacker can target their
shortRecord
to be redeemed and leave its correspondingshortOrder
withercAmount
<minShortErc
to keep alive on the order book. Once their smallshortOrder
is matched again, it will leave theshortRecord
position less than theminShortErc
threshold, disincentivizing liquidators from liquidating it.Furthermore, the small
shortRecord
(i.e.,shortRecord.ercDebt
<minShortErc
) also disables the redemption mechanism from redeeming it forethCollateral
.@1 -- The shortOrder is loaded from storage according to the attacker's p.shortOrderId param
: https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L110@2 -- The attacker can leave their target partially-filled shortOrder (corresponding to the redeeming shortRecord) with ercAmount < minShortErc alive by specifying the p.shortOrderId param to another shortOrder's id with ercAmount >= minShortErc
: https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L111@3 -- The root cause is that the proposeRedemption() verifies the loaded shortOrder against the legitimate shortRecordId and shorter params only after the condition in @2 was met
: https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L112Tools Used
Manual Review
Recommended Mitigation Steps
To fix the vulnerability, move out the
shortOrder
verification check and execute it immediately after loading theshortOrder
from storage.Assessed type
Invalid Validation