From 88290c9dff3a0e2273498625f907947f94928fab Mon Sep 17 00:00:00 2001 From: woodser Date: Thu, 6 Jun 2024 09:33:38 -0400 Subject: [PATCH] fix invalid signature when sign offer re-requested after timeout --- .../haveno/core/offer/OpenOfferManager.java | 25 +++--- .../offer/placeoffer/PlaceOfferProtocol.java | 79 ++++++++++--------- .../tasks/MakerProcessSignOfferResponse.java | 5 +- .../java/haveno/core/trade/TradeManager.java | 2 +- .../ArbitratorProcessDepositRequest.java | 2 +- 5 files changed, 63 insertions(+), 50 deletions(-) diff --git a/core/src/main/java/haveno/core/offer/OpenOfferManager.java b/core/src/main/java/haveno/core/offer/OpenOfferManager.java index 2b5b074034..f507cd9758 100644 --- a/core/src/main/java/haveno/core/offer/OpenOfferManager.java +++ b/core/src/main/java/haveno/core/offer/OpenOfferManager.java @@ -230,9 +230,12 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe offerBookService.addOfferBookChangedListener(new OfferBookChangedListener() { @Override public void onAdded(Offer offer) { + + // cancel offer if reserved funds spent Optional openOfferOptional = getOpenOfferById(offer.getId()); if (openOfferOptional.isPresent() && openOfferOptional.get().getState() != OpenOffer.State.RESERVED && offer.isReservedFundsSpent()) { - closeOpenOffer(offer); + log.warn("Canceling open offer because reserved funds have been spent, offerId={}, state={}", offer.getId(), openOfferOptional.get().getState()); + cancelOpenOffer(openOfferOptional.get(), null, null); } } @Override @@ -552,7 +555,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe if (openOffer.isCanceled()) latch.countDown(); else { log.warn("Error processing unposted offer {}: {}", openOffer.getId(), errorMessage); - doCancel(openOffer); + doCancelOffer(openOffer); offer.setErrorMessage(errorMessage); latch.countDown(); errorMessageHandler.handleErrorMessage(errorMessage); @@ -622,19 +625,19 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe offerBookService.removeOffer(openOffer.getOffer().getOfferPayload(), () -> { ThreadUtils.submitToPool(() -> { // TODO: this runs off thread and then shows popup when done. should show overlay spinner until done - doCancel(openOffer); - resultHandler.handleResult(); + doCancelOffer(openOffer); + if (resultHandler != null) resultHandler.handleResult(); }); }, errorMessageHandler); } else { ThreadUtils.submitToPool(() -> { - doCancel(openOffer); - resultHandler.handleResult(); + doCancelOffer(openOffer); + if (resultHandler != null) resultHandler.handleResult(); }); } } else { - errorMessageHandler.handleErrorMessage("You can't remove an offer that is currently edited."); + if (errorMessageHandler != null) errorMessageHandler.handleErrorMessage("You can't remove an offer that is currently edited."); } } @@ -708,7 +711,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe } // remove open offer which thaws its key images - private void doCancel(@NotNull OpenOffer openOffer) { + private void doCancelOffer(@NotNull OpenOffer openOffer) { Offer offer = openOffer.getOffer(); offer.setState(Offer.State.REMOVED); openOffer.setState(OpenOffer.State.CANCELED); @@ -874,7 +877,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe if (scheduledOffer.getNumProcessingAttempts() >= MAX_PROCESS_ATTEMPTS) { log.warn("Offer canceled after {} attempts, offerId={}, error={}", scheduledOffer.getNumProcessingAttempts(), scheduledOffer.getId(), errorMessage); HavenoUtils.havenoSetup.getTopErrorMsg().set("Offer canceled after " + scheduledOffer.getNumProcessingAttempts() + " attempts. Please switch to a better Monero connection and try again.\n\nOffer ID: " + scheduledOffer.getId() + "\nError: " + errorMessage); - doCancel(scheduledOffer); + doCancelOffer(scheduledOffer); } errorMessages.add(errorMessage); } @@ -1754,7 +1757,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe } else { // cancel and recreate offer - doCancel(openOffer); + doCancelOffer(openOffer); Offer updatedOffer = new Offer(openOffer.getOffer().getOfferPayload()); updatedOffer.setPriceFeedService(priceFeedService); OpenOffer updatedOpenOffer = new OpenOffer(updatedOffer, openOffer.getTriggerPrice()); @@ -1770,7 +1773,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe }, (errorMessage) -> { if (!updatedOpenOffer.isCanceled()) { log.warn("Error reposting offer {}: {}", updatedOpenOffer.getId(), errorMessage); - doCancel(updatedOpenOffer); + doCancelOffer(updatedOpenOffer); updatedOffer.setErrorMessage(errorMessage); } latch.countDown(); diff --git a/core/src/main/java/haveno/core/offer/placeoffer/PlaceOfferProtocol.java b/core/src/main/java/haveno/core/offer/placeoffer/PlaceOfferProtocol.java index bb5480d0ac..e6b62fdc58 100644 --- a/core/src/main/java/haveno/core/offer/placeoffer/PlaceOfferProtocol.java +++ b/core/src/main/java/haveno/core/offer/placeoffer/PlaceOfferProtocol.java @@ -91,47 +91,54 @@ public class PlaceOfferProtocol { // TODO (woodser): switch to fluent public void handleSignOfferResponse(SignOfferResponse response, NodeAddress sender) { - log.debug("handleSignOfferResponse() " + model.getOpenOffer().getOffer().getId()); - model.setSignOfferResponse(response); + log.debug("handleSignOfferResponse() " + model.getOpenOffer().getOffer().getId()); + model.setSignOfferResponse(response); - if (!model.getOpenOffer().getOffer().getOfferPayload().getArbitratorSigner().equals(sender)) { - log.warn("Ignoring sign offer response from different sender"); - return; - } + // ignore if unexpected signer + if (!model.getOpenOffer().getOffer().getOfferPayload().getArbitratorSigner().equals(sender)) { + log.warn("Ignoring sign offer response from different sender"); + return; + } - // ignore if timer already stopped - if (timeoutTimer == null) { - log.warn("Ignoring sign offer response from arbitrator because timeout has expired for offer " + model.getOpenOffer().getOffer().getId()); - return; - } + // ignore if payloads have different timestamps + if (model.getOpenOffer().getOffer().getOfferPayload().getDate() != response.getSignedOfferPayload().getDate()) { + log.warn("Ignoring sign offer response from arbitrator for offer payload with different timestamp"); + return; + } - // reset timer - startTimeoutTimer(); + // ignore if timer already stopped + if (timeoutTimer == null) { + log.warn("Ignoring sign offer response from arbitrator because timeout has expired for offer " + model.getOpenOffer().getOffer().getId()); + return; + } - TaskRunner taskRunner = new TaskRunner<>(model, - () -> { - log.debug("sequence at handleSignOfferResponse completed"); - stopTimeoutTimer(); - resultHandler.handleResult(model.getTransaction()); // TODO (woodser): XMR transaction instead - }, - (errorMessage) -> { - if (model.isOfferAddedToOfferBook()) { - model.getOfferBookService().removeOffer(model.getOpenOffer().getOffer().getOfferPayload(), - () -> { - model.setOfferAddedToOfferBook(false); - log.debug("OfferPayload removed from offer book."); - }, - log::error); - } - handleError(errorMessage); - } - ); - taskRunner.addTasks( - MakerProcessSignOfferResponse.class, - AddToOfferBook.class - ); + // reset timer + startTimeoutTimer(); - taskRunner.run(); + TaskRunner taskRunner = new TaskRunner<>(model, + () -> { + log.debug("sequence at handleSignOfferResponse completed"); + stopTimeoutTimer(); + resultHandler.handleResult(model.getTransaction()); // TODO (woodser): XMR transaction instead + }, + (errorMessage) -> { + if (model.isOfferAddedToOfferBook()) { + model.getOfferBookService().removeOffer(model.getOpenOffer().getOffer().getOfferPayload(), + () -> { + model.setOfferAddedToOfferBook(false); + log.debug("OfferPayload removed from offer book."); + }, + log::error); + } + handleError(errorMessage); + } + ); + taskRunner.addTasks( + MakerProcessSignOfferResponse.class, + AddToOfferBook.class + ); + + taskRunner.run(); } public void startTimeoutTimer() { diff --git a/core/src/main/java/haveno/core/offer/placeoffer/tasks/MakerProcessSignOfferResponse.java b/core/src/main/java/haveno/core/offer/placeoffer/tasks/MakerProcessSignOfferResponse.java index 0b53c2473d..f8baf8a400 100644 --- a/core/src/main/java/haveno/core/offer/placeoffer/tasks/MakerProcessSignOfferResponse.java +++ b/core/src/main/java/haveno/core/offer/placeoffer/tasks/MakerProcessSignOfferResponse.java @@ -42,11 +42,14 @@ public class MakerProcessSignOfferResponse extends Task { // validate arbitrator signature if (!HavenoUtils.isArbitratorSignatureValid(model.getSignOfferResponse().getSignedOfferPayload(), arbitrator)) { - throw new RuntimeException("Offer payload has invalid arbitrator signature"); + throw new RuntimeException("Arbitrator's offer payload has invalid signature, offerId=" + offer.getId()); } // set arbitrator signature for maker's offer offer.getOfferPayload().setArbitratorSignature(model.getSignOfferResponse().getSignedOfferPayload().getArbitratorSignature()); + if (!HavenoUtils.isArbitratorSignatureValid(offer.getOfferPayload(), arbitrator)) { + throw new RuntimeException("Maker's offer payload has invalid signature, offerId=" + offer.getId()); + } offer.setState(Offer.State.AVAILABLE); complete(); } catch (Exception e) { diff --git a/core/src/main/java/haveno/core/trade/TradeManager.java b/core/src/main/java/haveno/core/trade/TradeManager.java index 14f85318fb..54480b9d04 100644 --- a/core/src/main/java/haveno/core/trade/TradeManager.java +++ b/core/src/main/java/haveno/core/trade/TradeManager.java @@ -364,7 +364,7 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi trade.onShutDownStarted(); } catch (Exception e) { if (e.getMessage() != null && e.getMessage().contains("Connection reset")) return; // expected if shut down with ctrl+c - log.warn("Error notifying {} {} that shut down started {}", getClass().getSimpleName(), trade.getId()); + log.warn("Error notifying {} {} that shut down started {}", trade.getClass().getSimpleName(), trade.getId()); e.printStackTrace(); } }); diff --git a/core/src/main/java/haveno/core/trade/protocol/tasks/ArbitratorProcessDepositRequest.java b/core/src/main/java/haveno/core/trade/protocol/tasks/ArbitratorProcessDepositRequest.java index 45ce638f80..a97beb7d61 100644 --- a/core/src/main/java/haveno/core/trade/protocol/tasks/ArbitratorProcessDepositRequest.java +++ b/core/src/main/java/haveno/core/trade/protocol/tasks/ArbitratorProcessDepositRequest.java @@ -221,7 +221,7 @@ public class ArbitratorProcessDepositRequest extends TradeTask { processModel.getP2PService().sendEncryptedDirectMessage(nodeAddress, pubKeyRing, response, new SendDirectMessageListener() { @Override public void onArrived() { - log.info("{} arrived: trading peer={}; offerId={}; uid={}", response.getClass().getSimpleName(), nodeAddress, trade.getId()); + log.info("{} arrived: trading peer={}; offerId={}; uid={}", response.getClass().getSimpleName(), nodeAddress, trade.getId(), trade.getUid()); } @Override public void onFault(String errorMessage) {