Fix EscrowFinish fulfillment and condition parsing (#512)

check if parsed fulfillment and condition in EscrowFinish are equivalent to the raw value when re-written before setting typed fields.

Co-authored-by: David Fuelling <[email protected]>

xrpl4j-core/src/main/java/org/xrpl/xrpl4j/model/transactions/EscrowFinish.java

@@ -228,9 +228,19 @@ default EscrowFinish normalizeCondition() {
  // In this case, we should try to read conditionRawValue to a Condition. If that fails, condition()
  // will remain empty, otherwise we will set condition().
  try {
- Condition condition = CryptoConditionReader.readCondition(
- BaseEncoding.base16().decode(conditionRawValue().get().toUpperCase(Locale.US))
- );
+ byte[] conditionRawValueBytes = BaseEncoding.base16()
+ .decode(conditionRawValue().get().toUpperCase(Locale.US));
+ Condition condition = CryptoConditionReader.readCondition(conditionRawValueBytes);
+
+ // CryptoConditionReader.readCondition can succeed if the first part of the condition raw value
+ // is a valid condition, but the raw value has extra bytes afterward. Here, we check that the parsed
+ // condition is equivalent to the raw value when re-written.
+ if (!Arrays.equals(CryptoConditionWriter.writeCondition(condition), conditionRawValueBytes)) {
+ logger.warn("EscrowFinish Condition was malformed: mismatch between raw value and parsed condition. " +
+ "conditionRawValue() will contain the condition value, but condition() will be empty.");
+ return this;
+ }
+
  return EscrowFinish.builder().from(this)
  .condition(condition)
  .build();
@@ -300,9 +310,19 @@ default EscrowFinish normalizeFulfillment() {
  // In this case, we should try to read fulfillmentRawValue to a Condition. If that fails, fulfillment()
  // will remain empty, otherwise we will set fulfillment().
  try {
- Fulfillment<?> fulfillment = CryptoConditionReader.readFulfillment(
- BaseEncoding.base16().decode(fulfillmentRawValue().get().toUpperCase(Locale.US))
- );
+ byte[] fulfillmentRawValueBytes = BaseEncoding.base16()
+ .decode(fulfillmentRawValue().get().toUpperCase(Locale.US));
+ Fulfillment<?> fulfillment = CryptoConditionReader.readFulfillment(fulfillmentRawValueBytes);
+
+ // CryptoConditionReader.readFulfillment can succeed if the first part of the fulfillment raw value
+ // is a valid fulfillment, but the raw value has extra bytes afterward. Here, we check that the parsed
+ // fulfillment is equivalent to the raw value when re-written.
+ if (!Arrays.equals(CryptoConditionWriter.writeFulfillment(fulfillment), fulfillmentRawValueBytes)) {
+ logger.warn("EscrowFinish Fulfillment was malformed: mismatch between raw value and parsed fulfillment. " +
+ "fulfillmentRawValue() will contain the fulfillment value, but fulfillment() will be empty.");
+ return this;
+ }
+
  return EscrowFinish.builder().from(this)
  .fulfillment(fulfillment)
  .build();
@@ -318,7 +338,7 @@ default EscrowFinish normalizeFulfillment() {
  }
 
  } catch (DerEncodingException e) {
- // This should never happen. CryptoconditionWriter.writeCondition errantly declares that it can throw
+ // This should never happen. CryptoconditionWriter.writeFulfillment errantly declares that it can throw
  // a DerEncodingException, but nowhere in its implementation does it throw.
  throw new RuntimeException(e);
  }

xrpl4j-core/src/test/java/org/xrpl/xrpl4j/model/transactions/EscrowFinishTest.java

@@ -28,6 +28,7 @@
 import com.google.common.primitives.UnsignedInteger;
 import com.ripple.cryptoconditions.Condition;
 import com.ripple.cryptoconditions.CryptoConditionReader;
+import com.ripple.cryptoconditions.CryptoConditionWriter;
 import com.ripple.cryptoconditions.Fulfillment;
 import com.ripple.cryptoconditions.PreimageSha256Condition;
 import com.ripple.cryptoconditions.PreimageSha256Fulfillment;
@@ -176,6 +177,29 @@ void normalizeWithNoConditionAndRawValueForMalformedCondition() {
  assertThat(actual.conditionRawValue()).isNotEmpty().get().isEqualTo("1234");
  }
 
+ /**
+ * This tests the case where conditionRawValue is present and is parseable to a Condition but when the
+ * parsed Condition is written to a byte array, the value differs from the conditionRawValue bytes. This
+ * can occur if the condition raw value contains a valid condition in the first 32 bytes, but also includes
+ * extra bytes afterward.
+ */
+ @Test
+ void normalizeConditionWithRawValueThatIsParseableButNotValid() {
+ EscrowFinish actual = EscrowFinish.builder()
+ .fee(XrpCurrencyAmount.ofDrops(1))
+ .account(Address.of("rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59Ba"))
+ .sequence(UnsignedInteger.ONE)
+ .owner(Address.of("rN7n7otQDd6FczFgLdSqtcsAUxDkw6fzRH"))
+ .offerSequence(UnsignedInteger.ZERO)
+ .conditionRawValue(GOOD_CONDITION_STR + GOOD_CONDITION_STR)
+ .build();
+
+ assertThat(actual.condition()).isEmpty();
+ assertThat(actual.conditionRawValue()).isNotEmpty().get().isEqualTo(
+ GOOD_CONDITION_STR + GOOD_CONDITION_STR
+ );
+ }
+
  @Test
  void normalizeWithNoConditionAndRawValueForBadHexLengthCondition() {
  EscrowFinish actual = EscrowFinish.builder()
@@ -284,6 +308,29 @@ void normalizeWithNoFulfillmentAndRawValueForMalformedFulfillment() {
  assertThat(actual.fulfillmentRawValue()).isNotEmpty().get().isEqualTo("1234");
  }
 
+ /**
+ * This tests the case where fulfillmentRawValue is present and is parseable to a Fulfillment<?> but when the
+ * parsed Fulfillment is written to a byte array, the value differs from the fulfillmentRawValue bytes. This
+ * can occur if the fulfillment raw value contains a valid fulfillment in the first 32 bytes, but also includes
+ * extra bytes afterward, such as in transaction 138543329687544CDAFCD3AB0DCBFE9C4F8E710397747BA7155F19426F493C8D.
+ */
+ @Test
+ void normalizeFulfillmentWithRawValueThatIsParseableButNotValid() {
+ EscrowFinish actual = EscrowFinish.builder()
+ .fee(XrpCurrencyAmount.ofDrops(1))
+ .account(Address.of("rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59Ba"))
+ .sequence(UnsignedInteger.ONE)
+ .owner(Address.of("rN7n7otQDd6FczFgLdSqtcsAUxDkw6fzRH"))
+ .offerSequence(UnsignedInteger.ZERO)
+ .fulfillmentRawValue(GOOD_FULFILLMENT_STR + GOOD_FULFILLMENT_STR)
+ .build();
+
+ assertThat(actual.fulfillment()).isEmpty();
+ assertThat(actual.fulfillmentRawValue()).isNotEmpty().get().isEqualTo(
+ GOOD_FULFILLMENT_STR + GOOD_FULFILLMENT_STR
+ );
+ }
+
  @Test
  void normalizeWithNoFulfillmentAndRawValueForBadHexLengthFulfillment() {
  EscrowFinish actual = EscrowFinish.builder()