Summary
The shortRecord.collateral amount is not counted/saved correctly causing that the shorter receives more collateral when he exits his short position using the ExitShortFacet::exitShortWallet() function.
Vulnerability Details
When the shorter match his order a shortRecord is created using the LibShortRecord::createShortRecord() function. The problem is that the short record creation is using the matchTotal.fillEth as the collateral parameter for the LibShortRecord::createShortRecord() function. Causing that user receives more collateral when the shorter exits his position using ExitShortFacet::exitShortWallet() function. Please see the next scenario:
Match orders:
InitialMargin: 500
Short order: Price: 10 ercAmount: 5
Bid order: Price: 10, ercAmount: 3. There is only one bid order in the order book.
Shorter opens his position and its matched with a bid order, then the fillErc will be 3 and the fillEth will be 30 (highestBid * fillErc == 10 * 3).
The collateral used is calculated as incomingSell.price * fillErc * initialMargin == 10 * 3 * 500 == 15000
matchTotal.fillEth == 30 and matchTotal.colUsed == 15000
Now, since there is not more bid orders, the matchIncomingShort() is executed, the collateral used (15000) is deacreased from the shorter and the collateral used is increased on the matchTotal.fillEth variable, so now the matchTotal.fillEth == 15030 (15000 collateral + 30 fillEth).
Then, the shortRecord is created and it uses the matchTotal.fillEth as the collateral amount, the LibShortRecord::createShortRecord() uses that parameter as the collateral amount, then it saves the collateral amount.
The problem is exactly that, the user is saving more collateral in his short record (15030 instead of 15000). When user exists his position he will receive more collateral (15030) than the used (15000).
Additionally, the ShortRecord struct specifies that the collateral parameter should be the calculation of price * ercAmount * initialMargin NOT price * ercAmount * initialMargin + ethFilled
Impact
Shorter will receive more collateral than used if he decides to exit form his position.
Tools used
Manual review
Recommendations
The short record creation should use the collateral used:
File: LibOrders.sol
function matchIncomingShort(
address asset,
STypes.Order memory incomingShort,
MTypes.Match memory matchTotal
) private returns (uint8 shortRecordId) {
AppStorage storage s = appStorage();
STypes.Asset storage Asset = s.asset[asset];
uint256 vault = Asset.vault;
STypes.Vault storage Vault = s.vault[vault];
s.vaultUser[vault][incomingShort.addr].ethEscrowed -= matchTotal.colUsed;
matchTotal.fillEth += matchTotal.colUsed;
SR status = incomingShort.ercAmount == 0 ? SR.FullyFilled : SR.PartialFill;
shortRecordId = LibShortRecord.createShortRecord(
asset,
incomingShort.addr,
status,
-- matchTotal.fillEth,
++ matchTotal.collUsed,
matchTotal.fillErc,
Asset.ercDebtRate,
Vault.zethYieldRate,
0
);
Vault.dittoMatchedShares += matchTotal.dittoMatchedShares;
-- Vault.zethCollateral += matchTotal.fillEth;
-- Asset.zethCollateral += matchTotal.fillEth;
++ Vault.zethCollateral += matchTotal.collUsed;
++ Asset.zethCollateral += matchTotal.collUsed;
Asset.ercDebt += matchTotal.fillErc;
}
File: BidOrdersFacet.sol
function matchlowestSell(
address asset,
STypes.Order memory lowestSell,
STypes.Order memory incomingBid,
MTypes.Match memory matchTotal
) private {
uint88 fillErc = incomingBid.ercAmount > lowestSell.ercAmount
? lowestSell.ercAmount
: incomingBid.ercAmount;
uint88 fillEth = lowestSell.price.mulU88(fillErc);
if (lowestSell.orderType == O.LimitShort) {
// Match short
uint88 colUsed = fillEth.mulU88(LibOrders.convertCR(lowestSell.initialMargin));
LibOrders.increaseSharesOnMatch(asset, lowestSell, matchTotal, colUsed);
uint88 shortFillEth = fillEth + colUsed;
matchTotal.shortFillEth += shortFillEth;
++ matchTotal.colUsed += colUsed;
// Saves gas when multiple shorts are matched
if (!matchTotal.ratesQueried) {
STypes.Asset storage Asset = s.asset[asset];
matchTotal.ratesQueried = true;
matchTotal.ercDebtRate = Asset.ercDebtRate;
matchTotal.zethYieldRate = s.vault[Asset.vault].zethYieldRate;
}
// Default enum is PartialFill
SR status;
if (incomingBid.ercAmount >= lowestSell.ercAmount) {
status = SR.FullyFilled;
}
if (lowestSell.shortRecordId > 0) {
// shortRecord has been partially filled before
LibShortRecord.fillShortRecord(
asset,
lowestSell.addr,
lowestSell.shortRecordId,
status,
-- shortFillEth,
++ colUsed,
fillErc,
matchTotal.ercDebtRate,
matchTotal.zethYieldRate
);
} else {
// shortRecord newly created
lowestSell.shortRecordId = LibShortRecord.createShortRecord(
asset,
lowestSell.addr,
status,
-- shortFillEth,
++ colUsed,
fillErc,
matchTotal.ercDebtRate,
matchTotal.zethYieldRate,
0
);
}
} else {
// Match ask
s.vaultUser[s.asset[asset].vault][lowestSell.addr].ethEscrowed += fillEth;
matchTotal.askFillErc += fillErc;
}
matchTotal.fillErc += fillErc;
matchTotal.fillEth += fillEth;
}
File: BidOrdersFacet.sol
function matchIncomingBid(
address asset,
STypes.Order memory incomingBid,
MTypes.Match memory matchTotal,
MTypes.BidMatchAlgo memory b
) private returns (uint88 ethFilled, uint88 ercAmountLeft) {
if (matchTotal.fillEth == 0) {
return (0, incomingBid.ercAmount);
}
STypes.Asset storage Asset = s.asset[asset];
uint256 vault = Asset.vault;
LibOrders.updateSellOrdersOnMatch(asset, b);
// If at least one short was matched
if (matchTotal.shortFillEth > 0) {
STypes.Vault storage Vault = s.vault[vault];
// Matched Shares
Vault.dittoMatchedShares += matchTotal.dittoMatchedShares;
// Yield Accounting
-- Vault.zethCollateral += matchTotal.shortFillEth;
-- Asset.zethCollateral += matchTotal.shortFillEth;
++ Vault.zethCollateral += matchTotal.colUsed;
++ Asset.zethCollateral += matchTotal.colUsed;
Asset.ercDebt += matchTotal.fillErc - matchTotal.askFillErc;
//@dev Approximates the startingShortId after bid is fully executed
O shortOrderType = s.shorts[asset][b.shortId].orderType;
O prevShortOrderType = s.shorts[asset][b.prevShortId].orderType;
uint80 prevShortPrice = s.shorts[asset][b.prevShortId].price;
if (shortOrderType != O.Cancelled && shortOrderType != O.Matched) {
Asset.startingShortId = b.shortId;
} else if (
prevShortOrderType != O.Cancelled && prevShortOrderType != O.Matched
&& prevShortPrice >= b.oraclePrice
) {
Asset.startingShortId = b.prevShortId;
} else {
if (b.isMovingFwd) {
Asset.startingShortId = s.shorts[asset][b.shortId].nextId;
} else {
Asset.startingShortId = s.shorts[asset][b.shortHintId].nextId;
}
}
}
// Match bid
s.vaultUser[vault][incomingBid.addr].ethEscrowed -= matchTotal.fillEth;
s.assetUser[asset][incomingBid.addr].ercEscrowed += matchTotal.fillErc;
return (matchTotal.fillEth, incomingBid.ercAmount);
}
