fix: reject aliased ids in allowance owner and delegating spender

hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/CryptoApproveAllowanceHandler.java

@@ -13,6 +13,7 @@
 import static com.hedera.node.app.hapi.utils.fee.FeeConstants.LONG_SIZE;
 import static com.hedera.node.app.hapi.utils.fee.FeeConstants.NFT_ALLOWANCE_SIZE;
 import static com.hedera.node.app.hapi.utils.fee.FeeConstants.TOKEN_ALLOWANCE_SIZE;
+import static com.hedera.node.app.service.token.impl.validators.AllowanceValidator.designatesPayer;
 import static com.hedera.node.app.service.token.impl.validators.AllowanceValidator.isDelegatingSpenderPresent;
 import static com.hedera.node.app.service.token.impl.validators.AllowanceValidator.isValidOwner;
 import static com.hedera.node.app.service.token.impl.validators.AllowanceValidator.validateAllowanceLimit;
@@ -529,8 +530,7 @@ private static Account getEffectiveOwnerAccount(
             @NonNull final AccountID payerId,
             @NonNull final ReadableAccountStore accountStore,
             @NonNull final ExpiryValidator expiryValidator) {
-        final var ownerNum = owner != null ? owner.accountNumOrElse(0L) : 0L;
-        if (ownerNum == 0 || ownerNum == payerId.accountNumOrThrow()) {
+        if (designatesPayer(owner, payerId)) {
             // The payer would have been modified in the same transaction for previous allowances
             // So, get it from modifications map.
             return accountStore.getAccountById(payerId);

hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/validators/AllowanceValidator.java

@@ -142,7 +142,7 @@ public static Account getEffectiveOwner(
             @NonNull final Account payer,
             @NonNull final ReadableAccountStore accountStore,
             @NonNull final ExpiryValidator expiryValidator) {
-        if (owner == null || owner.accountNumOrElse(0L) == 0L || owner.equals(payer.accountId())) {
+        if (designatesPayer(owner, payer.accountIdOrThrow())) {
             return payer;
         } else {
             // If owner is in modifications get the modified account from state
@@ -156,6 +156,19 @@ public static Account getEffectiveOwner(
         }
     }
 
+    /**
+     * Returns whether the allowance owner means the payer. Only an unset id does, per
+     * {@code crypto_approve_allowance.proto}; an alias or a zero account number names another account.
+     *
+     * @param owner the owner given in the allowance, if any
+     * @param payerId the payer of the transaction
+     * @return true if the allowance applies to the payer's own account
+     */
+    public static boolean designatesPayer(@Nullable final AccountID owner, @NonNull final AccountID payerId) {
+        requireNonNull(payerId);
+        return owner == null || owner.equals(AccountID.DEFAULT) || owner.equals(payerId);
+    }
+
     /**
      * Returns whether the given NFT allowance carries a (non-default) delegating spender. A delegated allowance may
      * only sub-delegate individual serial numbers on the owner's behalf; it must never add or remove the owner's
@@ -168,7 +181,15 @@ public static Account getEffectiveOwner(
      */
     public static boolean isDelegatingSpenderPresent(@NonNull final NftAllowance allowance) {
         requireNonNull(allowance, "allowance must not be null");
-        return allowance.hasDelegatingSpender()
-                && allowance.delegatingSpenderOrThrow().accountNumOrThrow() != 0;
+        if (!allowance.hasDelegatingSpender()) {
+            return false;
+        }
+        final var delegatingSpender = allowance.delegatingSpenderOrThrow();
+        if (delegatingSpender.equals(AccountID.DEFAULT)) {
+            return false;
+        }
+        // Only an explicit zero means "no delegating spender" - unlike the owner, whose zero pureChecks rejects
+        // outright. A caller can still send an alias here, rejected further down, so the number needs guarding.
+        return !delegatingSpender.hasAccountNum() || delegatingSpender.accountNumOrThrow() != 0L;
     }
 }

hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/validators/ApproveAllowanceValidator.java

@@ -11,6 +11,7 @@
 import static com.hedera.hapi.node.base.ResponseCodeEnum.NFT_IN_FUNGIBLE_TOKEN_ALLOWANCES;
 import static com.hedera.hapi.node.base.ResponseCodeEnum.SPENDER_ACCOUNT_SAME_AS_OWNER;
 import static com.hedera.hapi.node.base.ResponseCodeEnum.TOKEN_NOT_ASSOCIATED_TO_ACCOUNT;
+import static com.hedera.node.app.service.token.impl.util.TokenHandlerHelper.AccountIDType.NOT_ALIASED_ID;
 import static com.hedera.node.app.service.token.impl.util.TokenHandlerHelper.TokenValidations.PERMIT_PAUSED;
 import static com.hedera.node.app.service.token.impl.util.TokenHandlerHelper.getIfUsable;
 import static com.hedera.node.app.spi.workflows.HandleException.validateFalse;
@@ -31,7 +32,6 @@
 import com.hedera.node.app.service.token.ReadableNftStore;
 import com.hedera.node.app.service.token.ReadableTokenRelationStore;
 import com.hedera.node.app.service.token.ReadableTokenStore;
-import com.hedera.node.app.service.token.impl.util.TokenHandlerHelper;
 import com.hedera.node.app.spi.validation.ExpiryValidator;
 import com.hedera.node.app.spi.workflows.HandleContext;
 import com.hedera.node.config.data.HederaConfig;
@@ -203,7 +203,7 @@ private void validateNftAllowances(
                         expiryValidator,
                         INVALID_DELEGATING_SPENDER,
                         INVALID_DELEGATING_SPENDER,
-                        TokenHandlerHelper.AccountIDType.ALIASED_ID);
+                        NOT_ALIASED_ID);
 
                 if (allowance.hasApprovedForAll()) {
                     validateFalse(

hedera-node/hedera-token-service-impl/src/test/java/com/hedera/node/app/service/token/impl/test/validators/AllowanceValidatorTest.java

@@ -7,6 +7,7 @@
 import static com.hedera.node.app.service.token.impl.schemas.V0490TokenSchema.ACCOUNTS_STATE_ID;
 import static com.hedera.node.app.service.token.impl.validators.AllowanceValidator.aggregateApproveNftAllowances;
 import static com.hedera.node.app.service.token.impl.validators.AllowanceValidator.getEffectiveOwner;
+import static com.hedera.node.app.service.token.impl.validators.AllowanceValidator.isDelegatingSpenderPresent;
 import static com.hedera.node.app.service.token.impl.validators.AllowanceValidator.isValidOwner;
 import static com.hedera.node.app.spi.fixtures.workflows.ExceptionConditions.responseCode;
 import static org.assertj.core.api.Assertions.assertThat;
@@ -25,6 +26,7 @@
 import com.hedera.node.app.service.token.impl.validators.AllowanceValidator;
 import com.hedera.node.app.spi.validation.ExpiryValidator;
 import com.hedera.node.app.spi.workflows.HandleException;
+import com.hedera.pbj.runtime.io.buffer.Bytes;
 import java.util.ArrayList;
 import java.util.List;
 import org.junit.jupiter.api.BeforeEach;
@@ -87,6 +89,50 @@ void validatesOwner() {
         assertThat(isValidOwner(nftSl1, ownerId, nonFungibleToken)).isTrue();
     }
 
+    @Test
+    void aliasedOwnerIsNotTreatedAsAbsent() {
+        final var aliasedOwner =
+                AccountID.newBuilder().alias(Bytes.wrap("aliasNotAnAccountNum")).build();
+        assertThatThrownBy(() -> getEffectiveOwner(aliasedOwner, account, readableAccountStore, expiryValidator))
+                .isInstanceOf(HandleException.class)
+                .has(responseCode(INVALID_ALLOWANCE_OWNER_ID));
+    }
+
+    @Test
+    void zeroNumberedOwnerIsInvalidRatherThanAbsent() {
+        // Account numbers must be positive - pureChecks already rejects a zero-numbered owner with
+        // INVALID_ACCOUNT_ID - so such an id is an invalid reference, not an omitted field, and the
+        // payer fallback must not apply to it
+        final var zeroOwner = AccountID.newBuilder().accountNum(0L).build();
+        assertThatThrownBy(() -> getEffectiveOwner(zeroOwner, account, readableAccountStore, expiryValidator))
+                .isInstanceOf(HandleException.class)
+                .has(responseCode(INVALID_ALLOWANCE_OWNER_ID));
+    }
+
+    @Test
+    void recognizesDelegatingSpenderWithoutReadingItAsANumber() {
+        final var withAlias = NftAllowance.newBuilder()
+                .delegatingSpender(AccountID.newBuilder()
+                        .alias(Bytes.wrap("aliasNotAnAccountNum"))
+                        .build())
+                .build();
+        assertThat(isDelegatingSpenderPresent(withAlias)).isTrue();
+
+        final var withNumber =
+                NftAllowance.newBuilder().delegatingSpender(spenderId).build();
+        assertThat(isDelegatingSpenderPresent(withNumber)).isTrue();
+
+        assertThat(isDelegatingSpenderPresent(NftAllowance.DEFAULT)).isFalse();
+        assertThat(isDelegatingSpenderPresent(NftAllowance.newBuilder()
+                        .delegatingSpender(AccountID.DEFAULT)
+                        .build()))
+                .isFalse();
+        assertThat(isDelegatingSpenderPresent(NftAllowance.newBuilder()
+                        .delegatingSpender(AccountID.newBuilder().accountNum(0L).build())
+                        .build()))
+                .isFalse();
+    }
+
     @Test
     void getsEffectiveOwnerIfOwnerNullOrZero() {
         assertThat(getEffectiveOwner(null, account, readableAccountStore, expiryValidator))

hedera-node/hedera-token-service-impl/src/test/java/com/hedera/node/app/service/token/impl/test/validators/ApproveAllowanceValidatorTest.java

@@ -242,6 +242,30 @@ void cannotGrantEmptySerialNftAllowanceWithDelegatingSpender() {
                 .has(responseCode(EMPTY_ALLOWANCES));
     }
 
+    @Test
+    void cannotNameDelegatingSpenderByAlias() {
+        // Aliases are resolved for CryptoTransfer only, so an allowance must name its delegating spender by account
+        // number. This alias does map to an account in state, which an aliased lookup would happily resolve - the
+        // rejection below is what keeps that from being honored here.
+        assertThat(readableAccountStore.getAliasedAccountById(alias)).isNotNull();
+        assertThat(readableAccountStore.getAccountById(alias)).isNull();
+
+        givenApproveAllowanceTxn(
+                payerId,
+                false,
+                List.of(),
+                List.of(),
+                List.of(nftAllowance
+                        .copyBuilder()
+                        .delegatingSpender(alias)
+                        .approvedForAll(Boolean.FALSE)
+                        .build()));
+
+        assertThatThrownBy(() -> subject.validate(handleContext, account, readableAccountStore))
+                .isInstanceOf(HandleException.class)
+                .has(responseCode(INVALID_DELEGATING_SPENDER));
+    }
+
     @Test
     void failsWhenTokenNotAssociatedToAccount() {
         givenApproveAllowanceTxn(

hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/integration/AliasedAllowanceIdsTest.java

@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: Apache-2.0
+package com.hedera.services.bdd.suites.integration;
+
+import static com.hedera.services.bdd.junit.TestTags.INTEGRATION;
+import static com.hedera.services.bdd.junit.hedera.embedded.EmbeddedMode.CONCURRENT;
+import static com.hedera.services.bdd.spec.HapiSpec.hapiTest;
+import static com.hedera.services.bdd.spec.queries.QueryVerbs.getAliasedAccountInfo;
+import static com.hedera.services.bdd.spec.transactions.TxnVerbs.cryptoApproveAllowance;
+import static com.hedera.services.bdd.spec.transactions.TxnVerbs.cryptoCreate;
+import static com.hedera.services.bdd.spec.transactions.TxnVerbs.cryptoTransfer;
+import static com.hedera.services.bdd.spec.transactions.TxnVerbs.mintToken;
+import static com.hedera.services.bdd.spec.transactions.TxnVerbs.tokenCreate;
+import static com.hedera.services.bdd.spec.transactions.crypto.HapiCryptoTransfer.tinyBarsFromAccountToAlias;
+import static com.hedera.services.bdd.spec.utilops.UtilVerbs.doingContextual;
+import static com.hedera.services.bdd.spec.utilops.UtilVerbs.newKeyNamed;
+import static com.hedera.services.bdd.suites.HapiSuite.ONE_HBAR;
+import static com.hedera.services.bdd.suites.HapiSuite.ONE_HUNDRED_HBARS;
+import static com.hedera.services.bdd.suites.crypto.AutoCreateUtils.updateSpecFor;
+import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INVALID_ALLOWANCE_OWNER_ID;
+import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INVALID_DELEGATING_SPENDER;
+import static com.hederahashgraph.api.proto.java.TokenType.NON_FUNGIBLE_UNIQUE;
+
+import com.google.protobuf.ByteString;
+import com.hedera.services.bdd.junit.HapiTest;
+import com.hedera.services.bdd.junit.TargetEmbeddedMode;
+import com.hedera.services.bdd.spec.keys.KeyShape;
+import com.hedera.services.bdd.spec.utilops.mod.BodyMutation;
+import com.hederahashgraph.api.proto.java.AccountID;
+import com.hederahashgraph.api.proto.java.NftAllowance;
+import com.hederahashgraph.api.proto.java.TransactionBody;
+import java.util.List;
+import java.util.function.UnaryOperator;
+import java.util.stream.Stream;
+import org.junit.jupiter.api.DynamicTest;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.parallel.Execution;
+import org.junit.jupiter.api.parallel.ExecutionMode;
+
+/**
+ * Covers NFT allowances that name an account with an alias rather than an account number. Such an id is a reference
+ * to some other account, so it must resolve to one; only a genuinely unset owner falls back to the payer.
+ *
+ * <p>{@code CryptoApproveAllowanceHandler#preHandle} picks one of the owner and the delegating spender as the account
+ * that must sign, and leaves the other unexamined: with {@code approved_for_all} set it requires the owner, otherwise
+ * it requires the delegating spender.
+ */
+@Tag(INTEGRATION)
+@TargetEmbeddedMode(CONCURRENT)
+@Execution(ExecutionMode.SAME_THREAD)
+public class AliasedAllowanceIdsTest {
+
+    private static final String OWNER = "aliasedAllowanceOwner";
+    private static final String SPENDER = "aliasedAllowanceSpender";
+    private static final String DELEGATE = "aliasedAllowanceDelegate";
+    private static final String NFT = "aliasedAllowanceNft";
+    private static final String SUPPLY_KEY = "aliasedAllowanceSupplyKey";
+    private static final String DELEGATE_KEY = "aliasedAllowanceDelegateKey";
+
+    /** An EVM address matching no account - the shape an SDK sends when naming an account by alias. */
+    private static final ByteString EVM_ALIAS = ByteString.copyFrom(new byte[] {
+        (byte) 0xA0,
+        (byte) 0xA1,
+        (byte) 0xA2,
+        (byte) 0xA3,
+        (byte) 0xA4,
+        (byte) 0xA5,
+        (byte) 0xA6,
+        (byte) 0xA7,
+        (byte) 0xA8,
+        (byte) 0xA9,
+        (byte) 0xAA,
+        (byte) 0xAB,
+        (byte) 0xAC,
+        (byte) 0xAD,
+        (byte) 0xAE,
+        (byte) 0xAF,
+        (byte) 0xB0,
+        (byte) 0xB1,
+        (byte) 0xB2,
+        (byte) 0xB3
+    });
+
+    private static final AccountID ALIASED_ID =
+            AccountID.newBuilder().setAlias(EVM_ALIAS).build();
+
+    /**
+     * A delegated allowance whose owner is named by alias fails with {@code INVALID_ALLOWANCE_OWNER_ID}: the alias
+     * belongs to no account, and an owner that is present but unresolvable is not the payer.
+     */
+    @HapiTest
+    Stream<DynamicTest> aliasedOwnerIsRejected() {
+        return hapiTest(
+                newKeyNamed(SUPPLY_KEY),
+                cryptoCreate(SPENDER).balance(ONE_HUNDRED_HBARS),
+                cryptoCreate(DELEGATE).balance(ONE_HUNDRED_HBARS),
+                cryptoCreate(OWNER).balance(ONE_HUNDRED_HBARS),
+                tokenCreate(NFT)
+                        .tokenType(NON_FUNGIBLE_UNIQUE)
+                        .initialSupply(0)
+                        .supplyKey(SUPPLY_KEY)
+                        .treasury(OWNER),
+                mintToken(NFT, List.of(ByteString.copyFromUtf8("a"))),
+                // Grant the delegate approveForAll over the payer's NFTs, so that every other requirement of the
+                // delegated allowance below is met - the delegate may sub-delegate the payer's serials, and the payer
+                // holds serial 1. The aliased owner is then the only thing the resulting status can be attributed to.
+                cryptoApproveAllowance().payingWith(OWNER).addNftAllowance(OWNER, NFT, DELEGATE, true, List.of()),
+                cryptoApproveAllowance()
+                        .payingWith(OWNER)
+                        .addDelegatedNftAllowance(OWNER, NFT, SPENDER, DELEGATE, false, List.of(1L))
+                        .withBodyMutation(BodyMutation.withTransform(mutatingNftAllowance(a -> a.setOwner(ALIASED_ID))))
+                        .signedBy(OWNER, DELEGATE)
+                        .hasKnownStatus(INVALID_ALLOWANCE_OWNER_ID));
+    }
+
+    /**
+     * A delegating spender named by alias must name an account, so an alias belonging to none fails with
+     * {@code INVALID_DELEGATING_SPENDER}.
+     */
+    @HapiTest
+    Stream<DynamicTest> aliasedDelegatingSpenderIsRejected() {
+        return hapiTest(
+                newKeyNamed(SUPPLY_KEY),
+                cryptoCreate(SPENDER).balance(ONE_HUNDRED_HBARS),
+                cryptoCreate(OWNER).balance(ONE_HUNDRED_HBARS),
+                tokenCreate(NFT)
+                        .tokenType(NON_FUNGIBLE_UNIQUE)
+                        .initialSupply(0)
+                        .supplyKey(SUPPLY_KEY)
+                        .treasury(OWNER),
+                cryptoApproveAllowance()
+                        .payingWith(OWNER)
+                        .addNftAllowance(OWNER, NFT, SPENDER, true, List.of())
+                        .withBodyMutation(BodyMutation.withTransform(
+                                mutatingNftAllowance(a -> a.setDelegatingSpender(ALIASED_ID))))
+                        .signedBy(OWNER)
+                        .hasKnownStatus(INVALID_DELEGATING_SPENDER));
+    }
+
+    /**
+     * The same rejection when the alias does resolve. The delegate here is a real account that the alias map really
+     * points at, and it is still refused, because allowances look ids up by account number and never consult that map.
+     *
+     * <p>{@code approved_for_all} is set so that pre-handle requires the owner's signature and leaves the delegating
+     * spender unexamined; that is what lets the aliased id reach handle, where the lookup happens.
+     */
+    @HapiTest
+    Stream<DynamicTest> resolvableAliasedDelegatingSpenderIsRejected() {
+        return hapiTest(
+                newKeyNamed(SUPPLY_KEY),
+                newKeyNamed(DELEGATE_KEY).shape(KeyShape.ED25519),
+                cryptoCreate(SPENDER).balance(ONE_HUNDRED_HBARS),
+                cryptoCreate(OWNER).balance(ONE_HUNDRED_HBARS),
+                // Auto-creation is what puts an alias in state; the created account's alias is its serialized key,
+                // and the query below fails outright unless that alias really resolves
+                cryptoTransfer(tinyBarsFromAccountToAlias(OWNER, DELEGATE_KEY, ONE_HBAR)),
+                doingContextual(spec -> updateSpecFor(spec, DELEGATE_KEY)),
+                getAliasedAccountInfo(DELEGATE_KEY),
+                tokenCreate(NFT)
+                        .tokenType(NON_FUNGIBLE_UNIQUE)
+                        .initialSupply(0)
+                        .supplyKey(SUPPLY_KEY)
+                        .treasury(OWNER),
+                cryptoApproveAllowance()
+                        .payingWith(OWNER)
+                        .addNftAllowance(OWNER, NFT, SPENDER, true, List.of())
+                        .withBodyMutation((builder, spec) ->
+                                mutatingNftAllowance(a -> a.setDelegatingSpender(AccountID.newBuilder()
+                                                .setAlias(spec.registry()
+                                                        .getKey(DELEGATE_KEY)
+                                                        .toByteString())
+                                                .build()))
+                                        .apply(builder.build())
+                                        .toBuilder())
+                        .signedBy(OWNER)
+                        .hasKnownStatus(INVALID_DELEGATING_SPENDER));
+    }
+
+    /** Rewrites the transaction's single NFT allowance, which the DSL cannot express with an aliased id. */
+    private static UnaryOperator<TransactionBody> mutatingNftAllowance(
+            final UnaryOperator<NftAllowance.Builder> mutation) {
+        return body -> {
+            final var op = body.getCryptoApproveAllowance().toBuilder();
+            op.setNftAllowances(0, mutation.apply(op.getNftAllowances(0).toBuilder()));
+            return body.toBuilder().setCryptoApproveAllowance(op).build();
+        };
+    }
+}