diff --git a/hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/CryptoApproveAllowanceHandler.java b/hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/CryptoApproveAllowanceHandler.java index 34c2266cc336..9e675b20e538 100644 --- a/hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/CryptoApproveAllowanceHandler.java +++ b/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); diff --git a/hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/validators/AllowanceValidator.java b/hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/validators/AllowanceValidator.java index 553ccf35491a..320dee83c7ca 100644 --- a/hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/validators/AllowanceValidator.java +++ b/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; } } diff --git a/hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/validators/ApproveAllowanceValidator.java b/hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/validators/ApproveAllowanceValidator.java index 6ea76d0e90db..42cd3360e576 100644 --- a/hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/validators/ApproveAllowanceValidator.java +++ b/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( diff --git a/hedera-node/hedera-token-service-impl/src/test/java/com/hedera/node/app/service/token/impl/test/validators/AllowanceValidatorTest.java b/hedera-node/hedera-token-service-impl/src/test/java/com/hedera/node/app/service/token/impl/test/validators/AllowanceValidatorTest.java index 8ab881ab25b5..6752f2f24e32 100644 --- a/hedera-node/hedera-token-service-impl/src/test/java/com/hedera/node/app/service/token/impl/test/validators/AllowanceValidatorTest.java +++ b/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)) diff --git a/hedera-node/hedera-token-service-impl/src/test/java/com/hedera/node/app/service/token/impl/test/validators/ApproveAllowanceValidatorTest.java b/hedera-node/hedera-token-service-impl/src/test/java/com/hedera/node/app/service/token/impl/test/validators/ApproveAllowanceValidatorTest.java index 2522abde22f6..3649a63605d6 100644 --- a/hedera-node/hedera-token-service-impl/src/test/java/com/hedera/node/app/service/token/impl/test/validators/ApproveAllowanceValidatorTest.java +++ b/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( diff --git a/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/integration/AliasedAllowanceIdsTest.java b/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/integration/AliasedAllowanceIdsTest.java new file mode 100644 index 000000000000..7014ce8c02d7 --- /dev/null +++ b/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. + * + *

{@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 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 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. + * + *

{@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 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 mutatingNftAllowance( + final UnaryOperator mutation) { + return body -> { + final var op = body.getCryptoApproveAllowance().toBuilder(); + op.setNftAllowances(0, mutation.apply(op.getNftAllowances(0).toBuilder())); + return body.toBuilder().setCryptoApproveAllowance(op).build(); + }; + } +}