search

sherlock-audit / 2023-06-symmetrical-judging

5 stars 4 forks source link

xiaoming90 - Rounding error when closing quote #251

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

xiaoming90

medium

Rounding error when closing quote

Summary

Rounding errors could occur if the provided filledAmount is too small, resulting in the locked balance of an account remains the same even though a certain amount of the position has been closed.

Vulnerability Detail

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/libraries/LibQuote.sol#L155

File: LibQuote.sol
149:     function closeQuote(Quote storage quote, uint256 filledAmount, uint256 closedPrice) internal {
150:         QuoteStorage.Layout storage quoteLayout = QuoteStorage.layout();
151:         AccountStorage.Layout storage accountLayout = AccountStorage.layout();
152: 
153:         quote.modifyTimestamp = block.timestamp;
154: 
155:         LockedValues memory lockedValues = LockedValues(
156:             quote.lockedValues.cva -
157:                 ((quote.lockedValues.cva * filledAmount) / (LibQuote.quoteOpenAmount(quote))),
158:             quote.lockedValues.mm -
159:                 ((quote.lockedValues.mm * filledAmount) / (LibQuote.quoteOpenAmount(quote))),
160:             quote.lockedValues.lf -
161:                 ((quote.lockedValues.lf * filledAmount) / (LibQuote.quoteOpenAmount(quote)))
162:         );
163:         accountLayout.lockedBalances[quote.partyA].subQuote(quote).add(lockedValues);
164:         accountLayout.partyBLockedBalances[quote.partyB][quote.partyA].subQuote(quote).add(
165:             lockedValues
166:         );
167:         quote.lockedValues = lockedValues;
168: 
169:         (bool hasMadeProfit, uint256 pnl) = LibQuote.getValueOfQuoteForPartyA(
170:             closedPrice,
171:             filledAmount,
172:             quote
173:         );
174:         if (hasMadeProfit) {
175:             accountLayout.allocatedBalances[quote.partyA] += pnl;
176:             accountLayout.partyBAllocatedBalances[quote.partyB][quote.partyA] -= pnl;
177:         } else {
178:             accountLayout.allocatedBalances[quote.partyA] -= pnl;
179:             accountLayout.partyBAllocatedBalances[quote.partyB][quote.partyA] += pnl;
180:         }

In Lines 157, 159, and 161 above, a malicious user could make the numerator smaller than the denominator (LibQuote.quoteOpenAmount(quote)), and the result will be zero due to a rounding error in Solidity.

In this case, the quote.lockedValues will not decrease and will remain the same. As a result, the locked balance of the account will remain the same even though a certain amount of the position has been closed. This could cause the account's locked balance to be higher than expected, and the errors will accumulate if it happens many times.

Impact

When an account's locked balances are higher than expected, their available balance will be lower than expected. The available balance affects the amount that users can withdraw from their accounts. The "silent" increase in their locked values means that the amount that users can withdraw becomes lesser over time, and these amounts are lost due to the errors.

Code Snippet

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/libraries/LibQuote.sol#L155

Tool used

Manual Review

Recommendation

When the ((quote.lockedValues.cva * filledAmount) / (LibQuote.quoteOpenAmount(quote))) rounds down to zero, this means that a rounding error has occurred as the numerator is smaller than the denominator. The CVA, filledAmount or both might be too small.

Consider performing input validation against the filledAmount within the fillCloseRequest function to ensure that the provided values are sufficiently large and will not result in a rounding error.

sherlock-admin2 commented 1 year ago

Escalate

Given that:

The amounts remaining locked are very small (magnitude of few wei per call), this issue should be of low severity per sherlock standards

You've deleted an escalation for this issue.
xiaoming9090 commented 1 year ago

Escalate

Escalate

Given that:

The amounts remaining locked are very small (magnitude of few wei per call), this issue should be of low severity per sherlock standards

Regarding the second bullet point, the checks will only be relevant if the position is fully closed via the forceClosePosition or emergencyClosePosition function, as they are forced to close the entire quantity of the position. However, if the position is partially filled via the fillCloseRequest, the error might still occur since the caller can specify an arbitrary filledAmount.

If an account has only traded a few times and the error only happens a few times, it might be fine. However, this is a trading protocol, and it is expected that there will be users who are active traders or entities that perform high-frequency or algorithmic trading.

Thus, the error will eventually accumulate into a significant value for these types of traders, and they will suffer the consequences as mentioned in my impact section of the report. In addition, it breaks an important protocol invariant where the locked balance does not decrease when a position is closed, which is unacceptable.

sherlock-admin2 commented 1 year ago

Escalate

Escalate

Given that:

The amounts remaining locked are very small (magnitude of few wei per call), this issue should be of low severity per sherlock standards

Regarding the second bullet point, the checks will only be relevant if the position is fully closed via the forceClosePosition or emergencyClosePosition function, as they are forced to close the entire quantity of the position. However, if the position is partially filled via the fillCloseRequest, the error might still occur since the caller can specify an arbitrary filledAmount.

If an account has only traded a few times and the error only happens a few times, it might be fine. However, this is a trading protocol, and it is expected that there will be users who are active traders or entities that perform high-frequency or algorithmic trading.

Thus, the error will eventually accumulate into a significant value for these types of traders, and they will suffer the consequences as mentioned in my impact section of the report. In addition, it breaks an important protocol invariant where the locked balance does not decrease when a position is closed, which is unacceptable.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

SergeKireev commented 1 year ago

Escalate

This issue is invalid because LibQuote.quoteOpenAmount(quote)) is defined as:

function quoteOpenAmount(Quote storage quote) internal view returns (uint256) {
    return quote.quantity - quote.closedAmount;
}

in the closeQuote function, quote.closedAmount is also updated as follows:

quote.closedAmount += filledAmount;

Which means that when the user closes the position completely filledAmount == LibQuote.quoteOpenAmount(quote)) (which also means filledAmount/LibQuote.quoteOpenAmount(quote)) == 1 and without rounding error) and the whole amount is unlocked in any case

sherlock-admin2 commented 1 year ago

Escalate

This issue is invalid because LibQuote.quoteOpenAmount(quote)) is defined as:

function quoteOpenAmount(Quote storage quote) internal view returns (uint256) {
    return quote.quantity - quote.closedAmount;
}

in the closeQuote function, quote.closedAmount is also updated as follows:

quote.closedAmount += filledAmount;

Which means that when the user closes the position completely filledAmount == LibQuote.quoteOpenAmount(quote)) (which also means filledAmount/LibQuote.quoteOpenAmount(quote)) == 1 and without rounding error) and the whole amount is unlocked in any case

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Evert0x commented 1 year ago

@xiaoming9090 any comment on the escalation by @SergeKireev ?

MoonKnightDev commented 1 year ago

Fixed code PR link: https://github.com/SYMM-IO/symmio-core/pull/24

xiaoming9090 commented 1 year ago

Escalate

This issue is invalid because LibQuote.quoteOpenAmount(quote)) is defined as:

function quoteOpenAmount(Quote storage quote) internal view returns (uint256) {
    return quote.quantity - quote.closedAmount;
}

in the closeQuote function, quote.closedAmount is also updated as follows:

quote.closedAmount += filledAmount;

Which means that when the user closes the position completely filledAmount == LibQuote.quoteOpenAmount(quote)) (which also means filledAmount/LibQuote.quoteOpenAmount(quote)) == 1 and without rounding error) and the whole amount is unlocked in any case

Disagree. This does not make the issue invalid. If the trader executes many small partial closes without completely closing the position, the accumulated error in the locked values will exist. It is unacceptable for the protocol to have inaccurate locked values at any single point in time. The locked values are used to determine many crucial decisions throughout the protocol, such as in assessing the solvency of an account.

hrishibhat commented 1 year ago

Result: Medium Has duplicates Agree with the fix and the comment here: https://github.com/sherlock-audit/2023-06-symmetrical-judging/issues/251#issuecomment-1685574052 maintaining severity as is

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status: