Skip to content

Commit

Permalink
Detect delegated sender authorization in CustomMessageCallProcessor (…
Browse files Browse the repository at this point in the history 
…#9814)

Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
  • Loading branch information
@tinker-michaelj
tinker-michaelj committed Nov 14, 2023
1 parent 56449f0 commit c194954
Show file tree
Hide file tree
Showing 6 changed files with 99 additions and 19 deletions.
4 changes: 2 additions & 2 deletions ...com/hedera/node/app/service/contract/impl/exec/processors/CustomMessageCallProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}
}
Expand Down
41 changes: 41 additions & 0 deletions ...e-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/utils/FrameUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
*
* <p>That is, returns true if the frame's <i>parent</i> 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.
*
* <p>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
* <b>only</b> signatures of keys of type {@link com.hedera.hapi.node.base.Key.KeyOneOfType#DELEGATABLE_CONTRACT_ID}.
*
* <p>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());
}
Expand Down
12 changes: 12 additions & 0 deletions ...a/node/app/service/contract/impl/test/exec/processors/CustomMessageCallProcessorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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);

Expand All @@ -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);

Expand Down Expand Up @@ -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<MessageFrame> stack = new ArrayDeque<>();
stack.push(frame);
stack.push(frame);
given(frame.getMessageFrameStack()).willReturn(stack);
}
}
46 changes: 43 additions & 3 deletions ...c/test/java/com/hedera/node/app/service/contract/impl/test/exec/utils/FrameUtilsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -61,7 +66,7 @@ class FrameUtilsTest {
@Mock
private MessageFrame initialFrame;

private Deque<MessageFrame> stack = new ArrayDeque<>();
private final Deque<MessageFrame> stack = new ArrayDeque<>();

@Test
void throwsInConstructor() {
Expand All @@ -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);
Expand Down Expand Up @@ -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));
Expand Down
14 changes: 0 additions & 14 deletions ...ce-impl/src/main/java/com/hedera/node/app/service/token/impl/api/TokenServiceApiImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions ...ients/src/main/java/com/hedera/services/bdd/suites/contract/hapi/ContractCreateSuite.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down

0 comments on commit c194954

Please sign in to comment.