Skip to content
This repository has been archived by the owner on Jan 7, 2024. It is now read-only.

berndartmueller - Emergency position closing can be griefed by Party A #298

Closed
sherlock-admin opened this issue Jul 3, 2023 · 0 comments
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label High A valid High severity issue Reward A payout will be made for this issue

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Jul 3, 2023

berndartmueller

medium

Emergency position closing can be griefed by Party A

Summary

Party B's emergency position closing attempt via the emergencyClosePosition function can be front-run by Party A by tactically incrementing Party A's nonce, leading to the invalidation of the signature used by Party B and causing the emergency close to reverting.

Vulnerability Detail

Party B can emergency close (i.e., completely filling) an opened position via the emergencyClosePosition function in the PartyBFacetImpl library, carrying out this action at the current market price denoted by upnlSig.price. The provided struct parameter upnlSig is verified via the LibMuon.verifyPairUpnlAndPrice function to ensure the struct is valid and legitimate and the price is not manipulated/stale.

Internally, the verifyPairUpnlAndPrice function calculates the hash with one of the values being the current nonce of Party A (AccountStorage.layout().partyANonces[partyA]).

However, Party A can increase the nonce in the meantime, for example, by using the AccountFacet.allocate function to allocate a small amount of deposited funds. This results in an increased nonce of Party A. If Party A front-runs the emergency close by Party B and purposefully increases its nonce, the signature used by Party B is invalidated, causing the signature verification to fail and, thus, the emergency close to revert.

The only way to prevent this from happening during an emergency close is to pause the accounting globally for everyone so that the whenNotAccountingPaused modifier reverts and allocating (i.e., increasing Party A's nonce) is not possible for the duration of the emergency close. Please note that this emergency mode can be enabled either globally or for specific Party B addresses. This aforementioned issue is particularly significant in the latter scenario, as a global pause would affect all other Party A participants in the protocol.

Impact

Party A can prevent Party B from closing the position via the emergencyClosePosition function and thus keep the position open for longer to potentially profit from it if the price moves in Party A's favor (or prevent emergency closing due to other reasons).

Code Snippet

contracts/facets/PartyB/PartyBFacetImpl.sol#L313

File: PartyBFacetImpl.sol
309: function emergencyClosePosition(uint256 quoteId, PairUpnlAndPriceSig memory upnlSig) internal {
310:     AccountStorage.Layout storage accountLayout = AccountStorage.layout();
311:     Quote storage quote = QuoteStorage.layout().quotes[quoteId];
312:     require(quote.quoteStatus == QuoteStatus.OPENED, "PartyBFacet: Invalid state");
313:     LibMuon.verifyPairUpnlAndPrice(upnlSig, quote.partyB, quote.partyA, quote.symbolId);// @audit-issue internally uses Party A's nonce to verify the signature
314:     uint256 filledAmount = LibQuote.quoteOpenAmount(quote);
315:     quote.quantityToClose = filledAmount;
316:     quote.requestedClosePrice = upnlSig.price;
317:     LibSolvency.isSolventAfterClosePosition(quoteId, filledAmount, upnlSig.price, upnlSig);
318:     accountLayout.partyBNonces[quote.partyB][quote.partyA] += 1;
319:     accountLayout.partyANonces[quote.partyA] += 1;
320:     LibQuote.closeQuote(quote, filledAmount, upnlSig.price);
321: }

contracts/libraries/LibMuon.sol#L180

161: function verifyPairUpnlAndPrice(
162:     PairUpnlAndPriceSig memory upnlSig,
163:     address partyB,
164:     address partyA,
165:     uint256 symbolId
166: ) internal view {
167:     MuonStorage.Layout storage muonLayout = MuonStorage.layout();
168: //        require(
169: //            block.timestamp <= upnlSig.timestamp + muonLayout.upnlValidTime,
170: //            "LibMuon: Expired signature"
171: //        );
172:     bytes32 hash = keccak256(
173:         abi.encodePacked(
174:             muonLayout.muonAppId,
175:             upnlSig.reqId,
176:             address(this),
177:             partyB,
178:             partyA,
179:             AccountStorage.layout().partyBNonces[partyB][partyA],
180:             AccountStorage.layout().partyANonces[partyA], // @audit-issue uses Party A's nonce
181:             upnlSig.upnlPartyB,
182:             upnlSig.upnlPartyA,
183:             symbolId,
184:             upnlSig.price,
185:             upnlSig.timestamp,
186:             getChainId()
187:         )
188:     );
189:     verifyTSSAndGateway(hash, upnlSig.sigs, upnlSig.gatewaySignature);
190: }

Tool used

Manual Review

Recommendation

Consider limiting the usage of Party A's actions, which can increase the nonce, during the time the emergency mode is active.

Duplicate of #233

@github-actions github-actions bot added High A valid High severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Jul 10, 2023
@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label High A valid High severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

1 participant