From c1949547d5e54b3297c51b7931f9d452dc4d9168 Mon Sep 17 00:00:00 2001 From: Michael Tinker Date: Tue, 14 Nov 2023 12:08:36 -0600 Subject: [PATCH] Detect delegated sender authorization in `CustomMessageCallProcessor` (#9814) Signed-off-by: Michael Tinker --- .../CustomMessageCallProcessor.java | 4 +- .../contract/impl/exec/utils/FrameUtils.java | 41 +++++++++++++++++ .../CustomMessageCallProcessorTest.java | 12 +++++ .../impl/test/exec/utils/FrameUtilsTest.java | 46 +++++++++++++++++-- .../token/impl/api/TokenServiceApiImpl.java | 14 ------ .../contract/hapi/ContractCreateSuite.java | 1 + 6 files changed, 99 insertions(+), 19 deletions(-) diff --git a/hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/processors/CustomMessageCallProcessor.java b/hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/processors/CustomMessageCallProcessor.java index b335c460a2fe..1492365265bf 100644 --- a/hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/processors/CustomMessageCallProcessor.java +++ b/hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/processors/CustomMessageCallProcessor.java @@ -17,8 +17,8 @@ package com.hedera.node.app.service.contract.impl.exec.processors; import static com.hedera.node.app.service.contract.impl.exec.failure.CustomExceptionalHaltReason.INVALID_FEE_SUBMITTED; +import static com.hedera.node.app.service.contract.impl.exec.utils.FrameUtils.acquiredSenderAuthorizationViaDelegateCall; import static com.hedera.node.app.service.contract.impl.exec.utils.FrameUtils.alreadyHalted; -import static com.hedera.node.app.service.contract.impl.exec.utils.FrameUtils.isDelegateCall; import static com.hedera.node.app.service.contract.impl.exec.utils.FrameUtils.transfersValue; import static org.hyperledger.besu.evm.frame.ExceptionalHaltReason.INSUFFICIENT_GAS; import static org.hyperledger.besu.evm.frame.ExceptionalHaltReason.PRECOMPILE_ERROR; @@ -210,7 +210,7 @@ private void doTransferValueOrHalt( frame.getSenderAddress(), frame.getRecipientAddress(), frame.getValue().toLong(), - isDelegateCall(frame)); + acquiredSenderAuthorizationViaDelegateCall(frame)); maybeReasonToHalt.ifPresent(reason -> doHalt(frame, reason, operationTracer)); } } diff --git a/hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/utils/FrameUtils.java b/hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/utils/FrameUtils.java index 0b35835ea668..4b2076682e51 100644 --- a/hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/utils/FrameUtils.java +++ b/hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/utils/FrameUtils.java @@ -22,6 +22,7 @@ import com.hedera.node.app.service.contract.impl.exec.gas.SystemContractGasCalculator; import com.hedera.node.app.service.contract.impl.exec.gas.TinybarValues; +import com.hedera.node.app.service.contract.impl.exec.processors.CustomMessageCallProcessor; import com.hedera.node.app.service.contract.impl.infra.StorageAccessTracker; import com.hedera.node.app.service.contract.impl.state.ProxyWorldUpdater; import com.hedera.node.config.data.ContractsConfig; @@ -79,6 +80,46 @@ public static boolean hasValidatedActionSidecarsEnabled(@NonNull final MessageFr return initialFrameOf(frame).getContextVariable(SYSTEM_CONTRACT_GAS_GAS_CALCULATOR_VARIABLE); } + /** + * Returns true if the given frame achieved its sender authorization via a delegate call. + * + *

That is, returns true if the frame's parent was executing code via a + * {@code DELEGATECALL} (or chain of {@code DELEGATECALL}'s); and the delegated code + * contained a {@code CALL}, {@code CALLCODE}, or {@code DELEGATECALL} instruction. In + * this case, the frame's sender is the recipient of the parent frame; the same as if the + * parent frame directly initiated a call. But our {@link com.hedera.hapi.node.base.Key} + * types are designed to enforce stricter permissions here, even though the sender address + * is the same. + * + *

In particular, if a contract {@code 0xabcd} initiates a call directly, then it + * can "activate" the signature of any {@link com.hedera.hapi.node.base.Key.KeyOneOfType#CONTRACT_ID} + * or {@link com.hedera.hapi.node.base.Key.KeyOneOfType#DELEGATABLE_CONTRACT_ID} key needed + * to authorize the call. But if its delegated code initiates a call, then it should activate + * only signatures of keys of type {@link com.hedera.hapi.node.base.Key.KeyOneOfType#DELEGATABLE_CONTRACT_ID}. + * + *

We thus use this helper in the {@link CustomMessageCallProcessor} to detect when an + * initiated call is being initiated by delegated code; and enforce the stricter permissions. + * + * @param frame the frame to check + * @return true if the frame achieved its sender authorization via a delegate call + */ + public static boolean acquiredSenderAuthorizationViaDelegateCall(@NonNull final MessageFrame frame) { + final var iter = frame.getMessageFrameStack().iterator(); + // Always skip the current frame + final var executingFrame = iter.next(); + if (frame != executingFrame) { + // This should be impossible + throw new IllegalArgumentException( + "Only the executing frame should be tested for delegate sender authorization"); + } + if (!iter.hasNext()) { + // The current frame is the initial frame, and thus not initiated from delegated code + return false; + } + final var parent = iter.next(); + return isDelegateCall(parent); + } + public static boolean isDelegateCall(@NonNull final MessageFrame frame) { return !frame.getRecipientAddress().equals(frame.getContractAddress()); } diff --git a/hedera-node/hedera-smart-contract-service-impl/src/test/java/com/hedera/node/app/service/contract/impl/test/exec/processors/CustomMessageCallProcessorTest.java b/hedera-node/hedera-smart-contract-service-impl/src/test/java/com/hedera/node/app/service/contract/impl/test/exec/processors/CustomMessageCallProcessorTest.java index d20f6cfc7cd7..195bdcd91c69 100644 --- a/hedera-node/hedera-smart-contract-service-impl/src/test/java/com/hedera/node/app/service/contract/impl/test/exec/processors/CustomMessageCallProcessorTest.java +++ b/hedera-node/hedera-smart-contract-service-impl/src/test/java/com/hedera/node/app/service/contract/impl/test/exec/processors/CustomMessageCallProcessorTest.java @@ -38,6 +38,8 @@ import com.hedera.node.app.service.contract.impl.test.TestHelpers; import com.swirlds.config.api.Configuration; import edu.umd.cs.findbugs.annotations.NonNull; +import java.util.ArrayDeque; +import java.util.Deque; import java.util.Map; import java.util.Optional; import java.util.concurrent.atomic.AtomicBoolean; @@ -181,6 +183,7 @@ void haltsIfValueTransferFails() { givenHaltableFrame(isHalted); given(frame.getValue()).willReturn(Wei.ONE); given(frame.getWorldUpdater()).willReturn(proxyWorldUpdater); + givenExecutingFrame(); given(addressChecks.isPresent(RECEIVER_ADDRESS, frame)).willReturn(true); given(proxyWorldUpdater.tryTransfer(SENDER_ADDRESS, RECEIVER_ADDRESS, Wei.ONE.toLong(), true)) .willReturn(Optional.of(ExceptionalHaltReason.ILLEGAL_STATE_CHANGE)); @@ -251,6 +254,7 @@ void triesLazyCreationBeforeValueTransferIfRecipientMissing() { given(proxyWorldUpdater.tryLazyCreation(RECEIVER_ADDRESS, frame)).willReturn(Optional.empty()); given(proxyWorldUpdater.tryTransfer(SENDER_ADDRESS, RECEIVER_ADDRESS, Wei.ONE.toLong(), true)) .willReturn(Optional.empty()); + givenExecutingFrame(); subject.start(frame, operationTracer); @@ -264,6 +268,7 @@ void tracesFailedCreateResultAfterHaltedLazyCreation() { given(frame.getValue()).willReturn(Wei.ONE); given(frame.getWorldUpdater()).willReturn(proxyWorldUpdater); given(proxyWorldUpdater.tryLazyCreation(RECEIVER_ADDRESS, frame)).willReturn(Optional.of(INSUFFICIENT_GAS)); + givenExecutingFrame(); subject.start(frame, operationTracer); @@ -325,4 +330,11 @@ private void verifyHalt(@NonNull final ExceptionalHaltReason reason, final boole argThat(result -> isSameResult(new Operation.OperationResult(0, reason), result))); } } + + private void givenExecutingFrame() { + final Deque stack = new ArrayDeque<>(); + stack.push(frame); + stack.push(frame); + given(frame.getMessageFrameStack()).willReturn(stack); + } } diff --git a/hedera-node/hedera-smart-contract-service-impl/src/test/java/com/hedera/node/app/service/contract/impl/test/exec/utils/FrameUtilsTest.java b/hedera-node/hedera-smart-contract-service-impl/src/test/java/com/hedera/node/app/service/contract/impl/test/exec/utils/FrameUtilsTest.java index 22ea68ba5113..0b93819f6831 100644 --- a/hedera-node/hedera-smart-contract-service-impl/src/test/java/com/hedera/node/app/service/contract/impl/test/exec/utils/FrameUtilsTest.java +++ b/hedera-node/hedera-smart-contract-service-impl/src/test/java/com/hedera/node/app/service/contract/impl/test/exec/utils/FrameUtilsTest.java @@ -21,8 +21,13 @@ import static com.hedera.node.app.service.contract.impl.exec.utils.FrameUtils.accessTrackerFor; import static com.hedera.node.app.service.contract.impl.exec.utils.FrameUtils.configOf; import static com.hedera.node.app.service.contract.impl.test.TestHelpers.DEFAULT_CONFIG; +import static com.hedera.node.app.service.contract.impl.test.TestHelpers.EIP_1014_ADDRESS; +import static com.hedera.node.app.service.contract.impl.test.TestHelpers.NON_SYSTEM_LONG_ZERO_ADDRESS; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.BDDMockito.given; import com.hedera.node.app.service.contract.impl.exec.operations.utils.OpUtils; @@ -61,7 +66,7 @@ class FrameUtilsTest { @Mock private MessageFrame initialFrame; - private Deque stack = new ArrayDeque<>(); + private final Deque stack = new ArrayDeque<>(); @Test void throwsInConstructor() { @@ -70,6 +75,42 @@ void throwsInConstructor() { } } + @Test + void initialFrameIsNotDelegated() { + stack.push(initialFrame); + given(initialFrame.getMessageFrameStack()).willReturn(stack); + assertFalse(FrameUtils.acquiredSenderAuthorizationViaDelegateCall(initialFrame)); + } + + @Test + void onlyExecutingFrameCanBeEvaluatedForDelegateSenderAuthorization() { + stack.push(frame); + stack.push(initialFrame); + given(frame.getMessageFrameStack()).willReturn(stack); + assertThrows( + IllegalArgumentException.class, () -> FrameUtils.acquiredSenderAuthorizationViaDelegateCall(frame)); + } + + @Test + void childOfParentExecutingItsOwnCodeDoesNotAcquireSenderAuthorizationViaDelegateCall() { + given(initialFrame.getRecipientAddress()).willReturn(EIP_1014_ADDRESS); + given(initialFrame.getContractAddress()).willReturn(EIP_1014_ADDRESS); + stack.push(initialFrame); + stack.push(frame); + given(frame.getMessageFrameStack()).willReturn(stack); + assertFalse(FrameUtils.acquiredSenderAuthorizationViaDelegateCall(frame)); + } + + @Test + void childOfParentExecutingDelegateCodeDoesAcquireSenderAuthorizationViaDelegateCall() { + given(initialFrame.getRecipientAddress()).willReturn(EIP_1014_ADDRESS); + given(initialFrame.getContractAddress()).willReturn(NON_SYSTEM_LONG_ZERO_ADDRESS); + stack.push(initialFrame); + stack.push(frame); + given(frame.getMessageFrameStack()).willReturn(stack); + assertTrue(FrameUtils.acquiredSenderAuthorizationViaDelegateCall(frame)); + } + @Test void getsContextVariablesFromInitialFrameIfStackEmpty() { given(frame.getMessageFrameStack()).willReturn(stack); @@ -117,8 +158,7 @@ private void assertFor(final Class clazz) { constructor.newInstance(); } catch (final InvocationTargetException expected) { final var cause = expected.getCause(); - Assertions.assertTrue( - cause instanceof UnsupportedOperationException, String.format(UNEXPECTED_THROW, cause, clazz)); + assertTrue(cause instanceof UnsupportedOperationException, String.format(UNEXPECTED_THROW, cause, clazz)); return; } catch (final Exception e) { Assertions.fail(String.format(UNEXPECTED_THROW, e, clazz)); diff --git a/hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/api/TokenServiceApiImpl.java b/hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/api/TokenServiceApiImpl.java index a92cd66dae8a..6a420ab1feed 100644 --- a/hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/api/TokenServiceApiImpl.java +++ b/hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/api/TokenServiceApiImpl.java @@ -321,11 +321,6 @@ public boolean chargeNetworkFee( requireNonNull(payerId); final var payerAccount = lookupAccount("Payer", payerId); - logger.info( - "Charging network fee of {} tinybars to {} ({} balance)", - amount, - payerId, - payerAccount.tinybarBalance()); final var amountToCharge = Math.min(amount, payerAccount.tinybarBalance()); chargePayer(payerAccount, amountToCharge); // We may be charging for preceding child record fees, which are additive to the base fee @@ -412,11 +407,6 @@ private void chargePayer(@NonNull final Account payerAccount, final long amount) .copyBuilder() .tinybarBalance(currentBalance - amount) .build()); - logger.info( - "Payer {} balance changing from {} to {}", - payerAccount.accountIdOrThrow(), - currentBalance, - currentBalance - amount); } /** @@ -572,24 +562,20 @@ private void distributeToNetworkFundingAccounts(final long amount, @NonNull fina // We may have a rounding error, so we will first remove the node and staking rewards from the total, and then // whatever is left over goes to the funding account. long balance = amount; - logger.info("Distributing {} to funding accounts", balance); // We only pay node and staking rewards if the feature is enabled if (stakingConfig.isEnabled()) { final long nodeReward = (stakingConfig.feesNodeRewardPercentage() * amount) / 100; balance -= nodeReward; - logger.info("Distributing {} to node reward account {}", nodeReward, nodeRewardAccountID); payNodeRewardAccount(nodeReward); final long stakingReward = (stakingConfig.feesStakingRewardPercentage() * amount) / 100; balance -= stakingReward; - logger.info("Distributing {} to staking reward account {}", stakingReward, stakingRewardAccountID); payStakingRewardAccount(stakingReward); } // Whatever is left over goes to the funding account final var fundingAccount = lookupAccount("Funding", fundingAccountID); - logger.info("Distributing {} to funding account {}", balance, fundingAccountID); accountStore.put(fundingAccount .copyBuilder() .tinybarBalance(fundingAccount.tinybarBalance() + balance) diff --git a/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/contract/hapi/ContractCreateSuite.java b/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/contract/hapi/ContractCreateSuite.java index 44db7162b62e..07c34c29e5bd 100644 --- a/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/contract/hapi/ContractCreateSuite.java +++ b/hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/contract/hapi/ContractCreateSuite.java @@ -415,6 +415,7 @@ private HapiSpec revertsNonzeroBalance() { .then(contractCreate(EMPTY_CONSTRUCTOR_CONTRACT).balance(1L).hasKnownStatus(CONTRACT_REVERT_EXECUTED)); } + @HapiTest private HapiSpec delegateContractIdRequiredForTransferInDelegateCall() { final var justSendContract = "JustSend"; final var sendInternalAndDelegateContract = "SendInternalAndDelegate";