fix: EIP-155 support in isAuthorized/isAuthorizedRaw (#17586)

Need to support chain ids such that encoding them in the `v` part of the signature according to EIP-155 works for "large" chain ids, such as Hedera's own.

Fixes #17273

Signed-off-by: David S Bakin <117694041+david-bakin-sl@users.noreply.github.com>

hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/systemcontracts/has/isauthorized/IsAuthorizedCall.java

@@ -122,21 +122,23 @@ protected PricedResult encodedOutput(
     }
 
     /**
-     * The Ethereum world uses 65 byte EC signatures, our cryptography library uses 64 byte EC signatures.  The
-     * difference is the addition of an extra "parity" byte at the end of the 64 byte signature (used so that
-     * `ECRECOVER` can recover the public key (== Ethereum address) from the signature.
+     * The Ethereum world uses 65+ byte EC signatures, our cryptography library uses 64 byte EC signatures.  The
+     * difference is the addition of an extra "parity" field at the end of the 64 byte signature (used so that
+     * `ECRECOVER` can recover the public key (== Ethereum address) from the signature.  And, the chain id can
+     * be encoded in that field (per EIP-155) and if the chain id is large enough (like Hedera mainnet/testnet
+     * chain ids) that last field can be more than one byte.
      *
-     * This method is a shim for that mismatch. It strips the extra byte off any 65 byte EC signatures it finds.
+     * This method is a shim for that mismatch. It strips the extra bytes off any 65+ byte EC signatures it finds.
      *
-     * @param sigMap Signature map from user - possibly contains 65 byte EC signatures
+     * @param sigMap Signature map from user - possibly contains 65+ byte EC signatures
      * @return Signature map with only 64 byte EC signatures (and all else unchanged)
      */
     public @NonNull SignatureMap fixEcSignaturesInMap(@NonNull final SignatureMap sigMap) {
         final List<SignaturePair> newPairs = new ArrayList<>();
         for (var spair : sigMap.sigPair()) {
             if (spair.hasEcdsaSecp256k1()) {
                 final var ecSig = requireNonNull(spair.ecdsaSecp256k1());
-                if (ecSig.length() == 65) {
+                if (ecSig.length() > 64) {
                     spair = new SignaturePair(
                             spair.pubKeyPrefix(), new OneOf<>(SignatureOneOfType.ECDSA_SECP256K1, ecSig.slice(0, 64)));
                 }

hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/systemcontracts/has/isauthorizedraw/IsAuthorizedRawCall.java

@@ -41,6 +41,7 @@
 import java.math.BigInteger;
 import java.nio.ByteBuffer;
 import java.util.Arrays;
+import java.util.Map;
 import java.util.Optional;
 import java.util.function.Function;
 import org.apache.tuweni.bytes.Bytes;
@@ -51,6 +52,13 @@
 /** HIP-632 method: `isAuthorizedRaw` */
 public class IsAuthorizedRawCall extends AbstractCall {
 
+    private static final int EIP_155_V_MIN_LENGTH = 1;
+    private static final int EIP_155_V_MAX_LENGTH = 8; // we limit chainId to fit in a `long`
+    private static final int EC_SIGNATURE_WITHOUT_V_LENGTH = 64;
+    public static final int EC_SIGNATURE_MIN_LENGTH = EC_SIGNATURE_WITHOUT_V_LENGTH + EIP_155_V_MIN_LENGTH;
+    public static final int EC_SIGNATURE_MAX_LENGTH = EC_SIGNATURE_WITHOUT_V_LENGTH + EIP_155_V_MAX_LENGTH;
+    public static final int ED_SIGNATURE_LENGTH = 64;
+
     private final Address address;
     private final byte[] messageHash;
     private final byte[] signature;
@@ -92,13 +100,7 @@ public IsAuthorizedRawCall(
     public PricedResult execute(@NonNull final MessageFrame frame) {
         requireNonNull(frame, "frame");
 
-        // First things first: What kind of signature are we dealing with, thus what kind of account+key we need
-        final var signatureType =
-                switch (signature.length) {
-                    case 65 -> SignatureType.EC;
-                    case 64 -> SignatureType.ED;
-                    default -> SignatureType.INVALID;
-                };
+        final var signatureType = signatureTypeFromItsLength(signature);
 
         // Now we know how much gas this call will cost
         final long gasRequirement =
@@ -133,7 +135,7 @@ public PricedResult execute(@NonNull final MessageFrame frame) {
         if (!isValidAccount(accountNum, signatureType)) return bail.apply(INVALID_ACCOUNT_ID);
         final var account = requireNonNull(enhancement.nativeOperations().getAccount(accountNum));
 
-        // If ED then require a key on the account
+        // If ED, then require a key on the account
         final Optional<Key> key;
         if (signatureType == SignatureType.ED) {
             key = Optional.ofNullable(account.key());
@@ -220,16 +222,16 @@ public Optional<byte[]> formatEcrecoverInput(@NonNull final byte[] messageHash,
         // Signature:
         //   [ 0;  31]  r
         //   [32;  63]  s
-        //   [64;  64]  v (but possibly 0..1 instead of 27..28)
+        //   [64;  64+]  v (but possibly 0..1 instead of 27..28)
 
         // From evm.codes, input to ECRECOVER:
         //   [ 0;  31]  hash
         //   [32;  63]  v == recovery identifier ∈ {27,28}
         //   [64;  95]  r == x-value ∈ (0, secp256k1n);
         //   [96; 127]  s ∈ (0; sep256k1n ÷ 2 + 1)
 
-        if (messageHash.length != 32 || signature.length != 65) return Optional.empty();
-        final var ov = reverseV(signature[64]);
+        if (messageHash.length != 32 || signature.length <= 64) return Optional.empty();
+        final var ov = reverseV(signature);
         if (ov.isEmpty()) return Optional.empty();
 
         final byte[] result = new byte[128];
@@ -241,24 +243,32 @@ public Optional<byte[]> formatEcrecoverInput(@NonNull final byte[] messageHash,
         return Optional.of(result);
     }
 
+    private static final Map<BigInteger, Byte> knownVs = Map.of(
+            BigInteger.ZERO,
+            (byte) 27,
+            BigInteger.ONE,
+            (byte) 28,
+            BigInteger.valueOf(27),
+            (byte) 27,
+            BigInteger.valueOf(28),
+            (byte) 28);
+
+    private static final BigInteger BIG_35 = BigInteger.valueOf(35);
+
     /** Make sure v ∈ {27, 28} - but after EIP-155 it might come in with a chain id ... */
     @NonNull
-    public Optional<Byte> reverseV(final byte v) {
+    public Optional<Byte> reverseV(final byte[] signature) {
+        requireNonNull(signature);
+        if (signature.length <= 64) throw new IllegalArgumentException("Signature is too short");
 
-        // We're getting the recovery value from a signature where it is only given a byte.  So
-        // this isn't the EIP-155 recovery value where the chain id is encoded in it (it's too
-        // small for most chain ids).  But I'm not 100% sure of that ... so ...
+        final var v = new BigInteger(1, signature, 64, signature.length - 64);
 
-        // FUTURE: Determine if in fact input `v` _ever_ has a EIP-155 encoded chain id ...
+        if (knownVs.containsKey(v)) return Optional.of(knownVs.get(v));
+        if (BIG_35.compareTo(v) > 0) return Optional.empty(); // invalid
 
-        if (v == 0 || v == 1) return Optional.of((byte) (v + 27));
-        if (v == 27 || v == 28) return Optional.of(v);
-        if (v >= 35) {
-            // EIP-155 case (35 is magic number for encoding chain id)
-            final var parity = (v - 35) % 2;
-            return Optional.of((byte) (parity + 27));
-        }
-        return Optional.empty();
+        // v = {0,1} + (chainid * 2) + 35 thus parity is the opposite of the low order bit
+        var parity = !v.testBit(0);
+        return Optional.of((byte) (parity ? 28 : 27));
     }
 
     public boolean isValidAccount(final long accountNum, @NonNull final SignatureType signatureType) {
@@ -277,4 +287,14 @@ public boolean isValidAccount(final long accountNum, @NonNull final SignatureTyp
 
         return true;
     }
+
+    @NonNull
+    private SignatureType signatureTypeFromItsLength(@NonNull final byte[] signature) {
+        final var len = signature.length;
+
+        if (EC_SIGNATURE_MIN_LENGTH <= len && len <= EC_SIGNATURE_MAX_LENGTH) return SignatureType.EC;
+        else if (ED_SIGNATURE_LENGTH == len) return SignatureType.ED;
+
+        return SignatureType.INVALID;
+    }
 }

hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/utils/SignatureMapUtils.java

@@ -24,6 +24,7 @@
 import com.hedera.node.app.spi.signatures.SignatureVerifier;
 import com.hedera.pbj.runtime.OneOf;
 import edu.umd.cs.findbugs.annotations.NonNull;
+import java.math.BigInteger;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -64,21 +65,30 @@ public static SignatureMap preprocessEcdsaSignatures(@NonNull final SignatureMap
 
     /**
      * Check that the v value in an ECDSA signature matches the chain ID if it is greater than 35 per EIP 155
-     * @param ecSig
-     * @param chainId
+     * @param ecSig (must be >64 bytes)
+     * @param chainId chainId which is to match that encoded in `ecSig`
      * @return true if the v value matches the chain ID or is not relevant, false otherwise
+     * <p>
+     * "Not relevant" means the signature is not EIP-155 encoded, so that the `v` value is simply
+     * the parity bit.
+     * <p>
+     * There's discussion on actually how big the chainid can get.  Some argue for 256 bits, or
+     * maybe just a little less than 255 bits.  Others argue for different numbers.  `long` is
+     * generous now for everything at chainlist.org (though of course someone could get silly later).
+     *
      */
-    public static boolean validChainId(final byte[] ecSig, final int chainId) {
-        int v = 0;
-        for (int i = 64; i < ecSig.length; i++) {
-            v <<= 8;
-            v |= (ecSig[i] & 0xFF);
-        }
-        if (v >= 35) {
-            // See EIP 155 - https://eips.ethereum.org/EIPS/eip-155
-            final var chainIdParityZero = 35 + (chainId * 2);
-            return v == chainIdParityZero || v == chainIdParityZero + 1;
+    public static boolean validChainId(final byte[] ecSig, final long chainId) {
+        if (ecSig.length <= 64) throw new IllegalArgumentException("signature needs to be at least 65 bytes");
+        try {
+            long v = new BigInteger(+1, ecSig, 64, ecSig.length - 64).longValueExact();
+            if (v >= 35) {
+                // See EIP 155 - https://eips.ethereum.org/EIPS/eip-155
+                final var chainIdParityZero = 35 + (chainId * 2L);
+                return v == chainIdParityZero || v == chainIdParityZero + 1;
+            }
+            return true;
+        } catch (ArithmeticException e) {
+            throw new IllegalArgumentException("EIP-155 encoded chain id too large (longer than long)");
         }
-        return true;
     }
 }

hedera-node/hedera-smart-contract-service-impl/src/test/java/com/hedera/node/app/service/contract/impl/test/exec/systemcontracts/has/isauthorizedraw/IsAuthorizedRawCallTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2023-2024 Hedera Hashgraph, LLC
+ * Copyright (C) 2023-2025 Hedera Hashgraph, LLC
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -18,6 +18,9 @@
 
 import static com.hedera.hapi.node.base.ResponseCodeEnum.INVALID_ACCOUNT_ID;
 import static com.hedera.node.app.service.contract.impl.exec.scope.HederaNativeOperations.MISSING_ENTITY_NUMBER;
+import static com.hedera.node.app.service.contract.impl.exec.systemcontracts.has.isauthorizedraw.IsAuthorizedRawCall.EC_SIGNATURE_MAX_LENGTH;
+import static com.hedera.node.app.service.contract.impl.exec.systemcontracts.has.isauthorizedraw.IsAuthorizedRawCall.EC_SIGNATURE_MIN_LENGTH;
+import static com.hedera.node.app.service.contract.impl.exec.systemcontracts.has.isauthorizedraw.IsAuthorizedRawCall.ED_SIGNATURE_LENGTH;
 import static com.hedera.node.app.service.contract.impl.test.TestHelpers.APPROVED_HEADLONG_ADDRESS;
 import static com.hedera.node.app.service.contract.impl.test.TestHelpers.OWNER_ACCOUNT;
 import static com.hedera.node.app.service.contract.impl.test.TestHelpers.OWNER_ACCOUNT_NUM;
@@ -41,6 +44,7 @@
 import com.hedera.node.app.service.contract.impl.test.exec.systemcontracts.common.CallTestBase;
 import com.hedera.node.app.spi.signatures.SignatureVerifier;
 import edu.umd.cs.findbugs.annotations.NonNull;
+import java.math.BigInteger;
 import org.hyperledger.besu.evm.frame.MessageFrame.State;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -62,12 +66,22 @@ class IsAuthorizedRawCallTest extends CallTestBase {
 
     @BeforeEach
     void setup() {
-        given(attempt.systemContractGasCalculator()).willReturn(gasCalculator);
-        given(attempt.enhancement()).willReturn(mockEnhancement());
-        given(attempt.signatureVerifier()).willReturn(signatureVerifier);
+        lenient().when(attempt.systemContractGasCalculator()).thenReturn(gasCalculator);
+        lenient().when(attempt.enhancement()).thenReturn(mockEnhancement());
+        lenient().when(attempt.signatureVerifier()).thenReturn(signatureVerifier);
         lenient().when(frame.getRemainingGas()).thenReturn(10_000_000L);
     }
 
+    @Test
+    void sanityCheckSignatureLengths() {
+        // Sadly, though AssertJ has `.isBetween` it does _not_ have `.isNotBetween`, or a general
+        // way to negate assertions ...
+
+        final var tooShortForEC = ED_SIGNATURE_LENGTH < EC_SIGNATURE_MIN_LENGTH;
+        final var tooLongForEC = ED_SIGNATURE_LENGTH > EC_SIGNATURE_MAX_LENGTH;
+        assertTrue(tooShortForEC || tooLongForEC);
+    }
+
     @Test
     void revertsWithNoAccountAtAddress() {
         given(nativeOperations.resolveAlias(any())).willReturn(MISSING_ENTITY_NUMBER);
@@ -106,10 +120,26 @@ void anyNonNegativeAccountValidIfED(final long account) {
     }
 
     @ParameterizedTest
-    @CsvSource({"0,27", "1,28", "27,27", "28,28", "45,27", "46,28", "18,"})
-    void reverseVTest(final byte fromV, final Byte expectedV) {
+    @CsvSource({
+        "0,27",
+        "1,28",
+        "27,27",
+        "28,28",
+        "45,27",
+        "46,28",
+        "1000,28",
+        "1001,27",
+        "10000000,28",
+        "10000003,27",
+        "18,"
+    })
+    void reverseVTest(final long fromV, final Byte expectedV) {
         subject = getSubject(APPROVED_HEADLONG_ADDRESS);
-        final var v = subject.reverseV(fromV);
+
+        final var r = asBytes(fromV);
+        final var ecSig = new byte[64 + r.length];
+        System.arraycopy(r, 0, ecSig, 64, r.length);
+        final var v = subject.reverseV(ecSig);
         if (expectedV == null) assertTrue(v.isEmpty());
         else assertEquals(expectedV, v.get());
     }
@@ -118,4 +148,9 @@ void reverseVTest(final byte fromV, final Byte expectedV) {
     IsAuthorizedRawCall getSubject(@NonNull final Address address) {
         return new IsAuthorizedRawCall(attempt, address, messageHash, signature, customGasCalculator);
     }
+
+    @NonNull
+    byte[] asBytes(final long n) {
+        return BigInteger.valueOf(n).toByteArray();
+    }
 }

hedera-node/hedera-smart-contract-service-impl/src/test/java/com/hedera/node/app/service/contract/impl/test/utils/SignatureMapUtilsTest.java

@@ -19,12 +19,16 @@
 import static com.hedera.node.app.service.contract.impl.utils.SignatureMapUtils.preprocessEcdsaSignatures;
 import static com.hedera.node.app.service.contract.impl.utils.SignatureMapUtils.validChainId;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import com.hedera.hapi.node.base.SignatureMap;
 import com.hedera.hapi.node.base.SignaturePair;
 import com.hedera.pbj.runtime.io.buffer.Bytes;
+import java.math.BigInteger;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 /**
  * Tests for the SignatureMapUtils class.
@@ -87,10 +91,40 @@ void testCorrectChainId() {
         assertThat(validChainId(ecSig, chainId)).isTrue();
     }
 
+    @ParameterizedTest
+    @ValueSource(ints = {0, 1})
+    void testOutrageouslyLargeChainId(final int parity) {
+        final var kakarotStarknetSepoliaChainid = 0x34550b76e4065L; // as seen on chainlist.org
+        var v = 35 + parity + (kakarotStarknetSepoliaChainid * 2);
+        var vBytes = BigInteger.valueOf(v).toByteArray();
+
+        var ecSig = new byte[64 + vBytes.length];
+        System.arraycopy(vBytes, 0, ecSig, 64, vBytes.length);
+
+        assertThat(validChainId(ecSig, kakarotStarknetSepoliaChainid)).isTrue();
+        assertThat(validChainId(ecSig, kakarotStarknetSepoliaChainid - 1)).isFalse();
+    }
+
+    @Test
+    void testOutOfBoundsChainId() {
+        var vBytes = new BigInteger("1122334455667788AABB", 16).toByteArray();
+
+        var ecSig = new byte[64 + vBytes.length];
+        System.arraycopy(vBytes, 0, ecSig, 64, vBytes.length);
+
+        assertThatExceptionOfType(IllegalArgumentException.class).isThrownBy(() -> validChainId(ecSig, 123456L));
+    }
+
     @Test
     void testChainIdBelow35() {
         final var ecSig = new byte[65];
         ecSig[64] = 34;
         assertThat(validChainId(ecSig, 0)).isTrue();
     }
+
+    @Test
+    void testSignatureTooShort() {
+        final var ecSig = new byte[64];
+        assertThatExceptionOfType(IllegalArgumentException.class).isThrownBy(() -> validChainId(ecSig, 0));
+    }
 }