Skip to content

Latest commit

 

History

History
147 lines (112 loc) · 8.35 KB

053.md

File metadata and controls

147 lines (112 loc) · 8.35 KB
Raw

Careful Fiery Scallop

High

Unsafe casting of reserveAmount from uint256 to int256

Summary

Unsafe casting of reserveAmount from uint256 to int256, leading to an incorrect value. As a result, it could lead to DOS or a loss of assets, as shown in the report below.

Root Cause

  • Unsafe casting of reserveAmount from uint256 to int256, leading to an incorrect value

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

At Line 118 below, the code performs an unsafe downcasting from uint256 to int256. If the reserveAmount exceeds the maximum value of an int256 (2^255 - 1), the conversion will not revert automatically. Instead, it will silently result in an incorrect wrapped value.

Note: It is possible for the reserveAmount to exceed 2^255 - 1 within the protocol as the depositToReserveVault function does not restrict users from depositing an amount larger than 2^255 - 1 and accountLayout.reserveVault[partyB] is uint256.

https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/ForceActions/ForceActionsFacetImpl.sol#L118

File: ForceActionsFacetImpl.sol
113: 		if (partyBAvailableBalance >= 0) {
114: 			if (updatedPrices.length > 0) {
115: 				LibSettlement.settleUpnl(settlementSig, updatedPrices, msg.sender, true);
116: 			}
117: 			LibQuote.closeQuote(quote, quote.quantityToClose, closePrice);
118: 		} else if (partyBAvailableBalance + int256(reserveAmount) >= 0) {
119: 			uint256 available = uint256(-partyBAvailableBalance);
120: 			accountLayout.reserveVault[quote.partyB] -= available;
121: 			accountLayout.partyBAllocatedBalances[quote.partyB][quote.partyA] += available;
123: 			if (updatedPrices.length > 0) {
124: 				LibSettlement.settleUpnl(settlementSig, updatedPrices, msg.sender, true);
125: 			}
126: 			LibQuote.closeQuote(quote, quote.quantityToClose, closePrice);
127: 		} else {
128: 			accountLayout.reserveVault[quote.partyB] = 0;
129: 			accountLayout.partyBAllocatedBalances[quote.partyB][quote.partyA] += reserveAmount;
131: 			int256 diff = (int256(quote.quantityToClose) * (int256(closePrice) - int256(sig.currentPrice))) / 1e18;
132: 			if (quote.positionType == PositionType.LONG) {
133: 				diff = diff * -1;
134: 			}
135: 			isPartyBLiquidated = true;
136: 			upnlPartyB = sig.upnlPartyB + diff;
137: 			LibLiquidation.liquidatePartyB(quote.partyB, quote.partyA, upnlPartyB, block.timestamp);
138: 		}

The following demonstrates a scenario where a reserveAmount that is larger than int256 (2^255 - 1) resulting in the condition (partyBAvailableBalance + int256(reserveAmount)) at Line 118 above to be evaluated to be incorrect. When reserveAmount is a positive value of 2**256 - 100, it will become a negative value (-100) when downcasted from uint256 to int256, as shown below.

➜  ~ chisel
Welcome to Chisel! Type `!help` to show available commands.
➜ uint256 reserveAmount = 2**256 - 100;
➜ int256 reserveAmountDowncast = int256(reserveAmount)
➜ reserveAmountDowncast
Type: int256
├ Hex: 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff9c
├ Hex (full word): 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff9c
└ Decimal: -100
➜ int256 partyBAvailableBalance = -150
➜ int256 result = partyBAvailableBalance + reserveAmountDowncast
➜ result
Type: int256
├ Hex: 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff06
├ Hex (full word): 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff06
└ Decimal: -250

If there is no downcasting error, the condition at Line 118 should have been evaluated as True since:

partyBAvailableBalance + reserveAmount >= 0
-100 + (2**256 - 100) >= 0
2**256 >= 0
True

However, as shown in the chisel output above, the final result is a negative number (result = partyBAvailableBalance + reserveAmountDowncast = -250), which leads to the condition being incorrectly evaluated as false.

This PartyB, with a large amount of reserve (2**256 - 100), should allow any force close action against them to be executed smoothly (without reverting or being liquidated) by going into the second branch of the if-else branch (Code in Lines 118-126).

However, due to the downcasting error, the logic in the final-else branch is executed instead (Lines 128-137 above), which contains the code for liquidating the PartyB (Line 127 above). Eventually, it is likely that the liquidation against PartyB will revert as the availableBalance within the LibLiquidation.availableBalance function will likely be larger than 0. This also means that PartyA cannot force close this position due to this revert.

On the other hand, it is possible that the liquidation process will go through without a revert (If the availableBalance within liquidatePartyB function increases, but still remains as a negative value). In this case, the consequences are as follows:

  • PartyB's position, which should not have been liquidated, gets liquidated instead, leading to a loss to PartyB because a certain portion of the position or its locked values need to be given to the liquidator as liquidation fees.
  • The liquidator (which is the PartyA triggering the force closing) will receive more liquidation fees than expected, which is incorrect.

There is also another possible scenario where an incorrect reserveAmount value after downcast at Line 118 causes the condition at Line 118 to be wrongly evaluated, leading to the quote being closed instead of PartyB being liquidated.

https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/ForceActions/ForceActionsFacetImpl.sol#L118

File: ForceActionsFacetImpl.sol
118: 		} else if (partyBAvailableBalance + int256(reserveAmount) >= 0) {
119: 			uint256 available = uint256(-partyBAvailableBalance);
120: 			accountLayout.reserveVault[quote.partyB] -= available;
121: 			accountLayout.partyBAllocatedBalances[quote.partyB][quote.partyA] += available;
122: 			emit SharedEvents.BalanceChangePartyB(quote.partyB, quote.partyA, available, SharedEvents.BalanceChangeType.REALIZED_PNL_IN);
123: 			if (updatedPrices.length > 0) {
124: 				LibSettlement.settleUpnl(settlementSig, updatedPrices, msg.sender, true);
125: 			}
126: 			LibQuote.closeQuote(quote, quote.quantityToClose, closePrice);
127: 		} else {
128: 			accountLayout.reserveVault[quote.partyB] = 0;
129: 			accountLayout.partyBAllocatedBalances[quote.partyB][quote.partyA] += reserveAmount;
130: 			emit SharedEvents.BalanceChangePartyB(quote.partyB, quote.partyA, reserveAmount, SharedEvents.BalanceChangeType.REALIZED_PNL_IN);
131: 			int256 diff = (int256(quote.quantityToClose) * (int256(closePrice) - int256(sig.currentPrice))) / 1e18;
132: 			if (quote.positionType == PositionType.LONG) {
133: 				diff = diff * -1;
134: 			}
135: 			isPartyBLiquidated = true;
136: 			upnlPartyB = sig.upnlPartyB + diff;
137: 			LibLiquidation.liquidatePartyB(quote.partyB, quote.partyA, upnlPartyB, block.timestamp);
138: 		}

Impact

Following is the list of possible negative impacts due to unsafe casting, summarized from the above report:

  • PartyA cannot force close this position due to a revert
  • PartyB's position, which should not have been liquidated, gets liquidated instead, leading to a loss to PartyB because a certain portion of the position or its locked values need to be given to the liquidator as liquidation fees.
  • The liquidator (which is the PartyA triggering the force closing) will receive more liquidation fees than expected, which is incorrect.
  • Quote incorrectly being closed instead of PartyB being liquidated.

PoC

No response

Mitigation

Consider implementing the following measures to mitigate the issue:

  • Use safe casting libraries, such as the OpenZeppelin's SafeCast
  • Limit the maximum deposit to the reserve vault to be within the range of int256.