Repository: hashgraph/hedera-services
Issue: Contract memo size limit not enforced

Created by [185](https://github.com/swirlds/hedera-fpcomplete-audit/pull/185)

## Situation

Both the [ContractCreate](https://docs.hedera.com/guides/docs/hedera-api/smart-contracts/contractcreate) and the [ContractUpdate](https://docs.hedera.com/guides/docs/hedera-api/smart-contracts/contractupdate) operations include a `memo` field with a note.

> max 100 bytes
When the node receives a transaction from a client via the gRPC interface, this constraint appears to be enforced as a part of validations in the `TransactionHandler.validateTransactionPreConsensus` method with this concrete check at https://github.com/swirlds/services-hedera/blob/4dca8c55e200e70774f2906f44a1bbbd69944ef3/hedera-node/src/main/java/com/opencrowd/handler/TransactionHandler.java#L431

No corresponding checks were found for transactions when they come from other nodes and receive consensus (with service logic invoked as a part of `AwareProcessLogic.incorporateConsensusTxn`).

## Problem

This issue is not a problem as long all nodes can be trusted, but if we understand correctly, that will not be the case anymore in the future.

Changed files:
hedera-node/src/test/java/com/hedera/services/legacy/services/state/AwareProcessLogicTest.java
hedera-node/src/main/java/com/hedera/services/legacy/services/state/AwareProcessLogic.java

diff --git a/hedera-node/src/main/java/com/hedera/services/legacy/services/state/AwareProcessLogic.java b/hedera-node/src/main/java/com/hedera/services/legacy/services/state/AwareProcessLogic.java
index a388201209..082a796e7a 100644
--- a/hedera-node/src/main/java/com/hedera/services/legacy/services/state/AwareProcessLogic.java
+++ b/hedera-node/src/main/java/com/hedera/services/legacy/services/state/AwareProcessLogic.java
@@ -70,6 +70,7 @@ import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INVALID_SIGNAT
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INVALID_SIGNATURE_COUNT_MISMATCHING_KEY;
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INVALID_TRANSACTION_DURATION;
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.KEY_PREFIX_MISMATCH;
+import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.MEMO_TOO_LONG;
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.MODIFYING_IMMUTABLE_CONTRACT;
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.OK;
 import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.SUCCESS;
@@ -77,6 +78,9 @@ import static java.time.ZoneOffset.UTC;
 import static java.time.temporal.ChronoUnit.SECONDS;
 
 public class AwareProcessLogic implements ProcessLogic {
+
+	private static final int MEMO_SIZE_LIMIT = 100;
+
 	static Logger log = LogManager.getLogger(AwareProcessLogic.class);
 
 	private static final EnumSet<ResponseCodeEnum> SIG_RATIONALIZATION_ERRORS = EnumSet.of(
@@ -365,6 +369,8 @@ public class AwareProcessLogic implements ProcessLogic {
 			try {
 				if (!ctx.hfs().exists(fid)) {
 					record = ctx.contracts().getFailureTransactionRecord(txn, consensusTime, INVALID_FILE_ID);
+				} else if(isMemoTooLong(txn.getContractCreateInstance().getMemo())) {
+					record = ctx.contracts().getFailureTransactionRecord(txn, consensusTime, MEMO_TOO_LONG);
 				} else {
 					byte[] contractByteCode = ctx.hfs().cat(fid);
 					if (contractByteCode.length > 0) {
@@ -385,7 +391,11 @@ public class AwareProcessLogic implements ProcessLogic {
 			}
 		} else if (txn.hasContractUpdateInstance()) {
 			try {
-				record = ctx.contracts().updateContract(txn, consensusTime);
+				if (isMemoTooLong(txn.getContractUpdateInstance().getMemo())) {
+					record = ctx.contracts().getFailureTransactionRecord(txn, consensusTime, MEMO_TOO_LONG);
+				} else {
+					record = ctx.contracts().updateContract(txn, consensusTime);
+				}
 			} catch (Exception e) {
 				log.error("Error during update contract", e);
 			}
@@ -417,4 +427,8 @@ public class AwareProcessLogic implements ProcessLogic {
 		}
 		return record;
 	}
+
+	private boolean isMemoTooLong(String memo) {
+		return memo.length() > MEMO_SIZE_LIMIT;
+	}
 }
diff --git a/hedera-node/src/test/java/com/hedera/services/legacy/services/state/AwareProcessLogicTest.java b/hedera-node/src/test/java/com/hedera/services/legacy/services/state/AwareProcessLogicTest.java
index d37a61b18c..d2dec42c51 100644
--- a/hedera-node/src/test/java/com/hedera/services/legacy/services/state/AwareProcessLogicTest.java
+++ b/hedera-node/src/test/java/com/hedera/services/legacy/services/state/AwareProcessLogicTest.java
@@ -21,9 +21,36 @@ package com.hedera.services.legacy.services.state;
  */
 
 import com.hedera.services.context.ServicesContext;
+import com.hedera.services.context.TransactionContext;
+import com.hedera.services.context.domain.trackers.IssEventInfo;
+import com.hedera.services.fees.FeeCalculator;
+import com.hedera.services.fees.charging.TxnFeeChargingPolicy;
+import com.hedera.services.files.HederaFs;
+import com.hedera.services.ledger.HederaLedger;
+import com.hedera.services.ledger.accounts.BackingAccounts;
+import com.hedera.services.legacy.handler.SmartContractRequestHandler;
+import com.hedera.services.legacy.services.stats.HederaNodeStats;
+import com.hedera.services.records.AccountRecordsHistorian;
+import com.hedera.services.records.TxnIdRecentHistory;
+import com.hedera.services.security.ops.SystemOpAuthorization;
+import com.hedera.services.security.ops.SystemOpPolicies;
+import com.hedera.services.sigs.order.HederaSigningOrder;
+import com.hedera.services.sigs.order.SigningOrderResult;
+import com.hedera.services.state.merkle.MerkleAccount;
+import com.hedera.services.state.submerkle.SequenceNumber;
+import com.hedera.services.txns.TransitionLogicLookup;
+import com.hedera.services.txns.validation.OptionValidator;
+import com.hedera.services.utils.PlatformTxnAccessor;
 import com.hedera.test.utils.IdUtils;
+import com.hederahashgraph.api.proto.java.AccountID;
+import com.hederahashgraph.api.proto.java.ContractCreateTransactionBody;
+import com.hederahashgraph.api.proto.java.ContractUpdateTransactionBody;
+import com.hederahashgraph.api.proto.java.Duration;
+import com.hederahashgraph.api.proto.java.FileID;
+import com.hederahashgraph.api.proto.java.ResponseCodeEnum;
 import com.hederahashgraph.api.proto.java.TransactionBody;
 import com.hederahashgraph.api.proto.java.TransactionID;
+import com.hederahashgraph.api.proto.java.TransactionRecord;
 import com.swirlds.common.Address;
 import com.swirlds.common.AddressBook;
 import com.swirlds.common.Transaction;
@@ -31,12 +58,19 @@ import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.junit.platform.runner.JUnitPlatform;
 import org.junit.runner.RunWith;
 
 import java.time.Instant;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Optional;
 
+import static com.hedera.services.context.domain.trackers.IssEventStatus.NO_KNOWN_ISS;
+import static com.hedera.services.txns.diligence.DuplicateClassification.BELIEVED_UNIQUE;
+import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.MEMO_TOO_LONG;
 import static org.mockito.BDDMockito.*;
 
 @RunWith(JUnitPlatform.class)
@@ -45,12 +79,39 @@ class AwareProcessLogicTest {
 	Transaction platformTxn;
 	AddressBook book;
 	ServicesContext ctx;
+	TransactionContext txnCtx;
+	TransactionBody txnBody;
+	SmartContractRequestHandler contracts;
+	HederaFs hfs;
 
 	AwareProcessLogic subject;
 
 	@BeforeEach
 	public void setup() {
+		final Transaction txn = mock(Transaction.class);
+		final PlatformTxnAccessor txnAccessor = mock(PlatformTxnAccessor.class);
+		final HederaLedger ledger = mock(HederaLedger.class);
+		final AccountRecordsHistorian historian = mock(AccountRecordsHistorian.class);
+		final HederaSigningOrder keyOrder = mock(HederaSigningOrder.class);
+		final SigningOrderResult orderResult = mock(SigningOrderResult.class);
+		final HederaNodeStats stats = mock(HederaNodeStats.class);
+		final FeeCalculator fees = mock(FeeCalculator.class);
+		final TxnIdRecentHistory recentHistory = mock(TxnIdRecentHistory.class);
+		final Map<TransactionID, TxnIdRecentHistory> histories = mock(Map.class);
+		final BackingAccounts<AccountID, MerkleAccount> backingAccounts = mock(BackingAccounts.class);
+		final AccountID accountID = mock(AccountID.class);
+		final OptionValidator validator = mock(OptionValidator.class);
+		final TxnFeeChargingPolicy policy = mock(TxnFeeChargingPolicy.class);
+		final SystemOpPolicies policies = mock(SystemOpPolicies.class);
+		final TransitionLogicLookup lookup = mock(TransitionLogicLookup.class);
+		hfs = mock(HederaFs.class);
+
+		given(histories.get(any())).willReturn(recentHistory);
+
+		txnCtx = mock(TransactionContext.class);
 		ctx = mock(ServicesContext.class);
+		txnBody = mock(TransactionBody.class);
+		contracts = mock(SmartContractRequestHandler.class);
 		mockLog = mock(Logger.class);
 		platformTxn = new Transaction(com.hederahashgraph.api.proto.java.Transaction.newBuilder()
 				.setBodyBytes(TransactionBody.newBuilder()
@@ -69,6 +130,51 @@ class AwareProcessLogicTest {
 		given(book.getAddress(1)).willReturn(stakedAddress);
 		given(book.getAddress(666L)).willReturn(zeroStakeAddress);
 		given(ctx.addressBook()).willReturn(book);
+		given(ctx.ledger()).willReturn(ledger);
+		given(ctx.txnCtx()).willReturn(txnCtx);
+		given(ctx.recordsHistorian()).willReturn(historian);
+		given(ctx.backedKeyOrder()).willReturn(keyOrder);
+		given(ctx.stats()).willReturn(stats);
+		given(ctx.fees()).willReturn(fees);
+		given(ctx.txnHistories()).willReturn(histories);
+		given(ctx.backingAccounts()).willReturn(backingAccounts);
+		given(ctx.validator()).willReturn(validator);
+		given(ctx.txnChargingPolicy()).willReturn(policy);
+		given(ctx.systemOpPolicies()).willReturn(policies);
+		given(ctx.transitionLogic()).willReturn(lookup);
+		given(ctx.hfs()).willReturn(hfs);
+		given(ctx.contracts()).willReturn(contracts);
+
+		given(txnCtx.accessor()).willReturn(txnAccessor);
+		given(txnCtx.submittingNodeAccount()).willReturn(accountID);
+		given(txnCtx.isPayerSigKnownActive()).willReturn(true);
+		given(txnAccessor.getPlatformTxn()).willReturn(txn);
+
+		given(txn.getSignatures()).willReturn(Collections.emptyList());
+		given(keyOrder.keysForPayer(any(), any())).willReturn(orderResult);
+		given(keyOrder.keysForOtherParties(any(), any())).willReturn(orderResult);
+
+		final com.hederahashgraph.api.proto.java.Transaction signedTxn = mock(com.hederahashgraph.api.proto.java.Transaction.class);
+		final TransactionID txnId = mock(TransactionID.class);
+
+		given(txnAccessor.getSignedTxn()).willReturn(signedTxn);
+		given(txnAccessor.getTxn()).willReturn(txnBody);
+		given(signedTxn.hasSigs()).willReturn(false);
+		given(txnBody.getTransactionID()).willReturn(txnId);
+		given(txnBody.getTransactionValidDuration()).willReturn(Duration.getDefaultInstance());
+
+		given(recentHistory.currentDuplicityFor(anyLong())).willReturn(BELIEVED_UNIQUE);
+		given(backingAccounts.contains(any())).willReturn(true);
+
+		given(validator.isValidTxnDuration(anyLong())).willReturn(true);
+		given(validator.chronologyStatus(any(), any())).willReturn(ResponseCodeEnum.OK);
+		given(validator.isValidAutoRenewPeriod(any())).willReturn(true);
+
+		given(txnBody.getNodeAccountID()).willReturn(accountID);
+		given(policy.apply(any(), any())).willReturn(ResponseCodeEnum.OK);
+		given(policies.check(any())).willReturn(SystemOpAuthorization.AUTHORIZED);
+		given(lookup.lookupFor(any(), any())).willReturn(Optional.empty());
+		given(hfs.exists(any())).willReturn(true);
 
 		subject = new AwareProcessLogic(ctx);
 	}
@@ -106,4 +212,164 @@ class AwareProcessLogicTest {
 		// then:
 		verify(mockLog).error(argThat((String s) -> s.startsWith("Catastrophic invariant failure!")));
 	}
-}
\\ No newline at end of file
+
+
+	@Test
+	@DisplayName("incorporateConsensusTxn assigns a failure due to memo size for ContractCreateInstance")
+	public void shortCircuitsOnMemoSizeForContractCreate() {
+		// setup:
+		final Instant now = Instant.now();
+		final Instant then = now.minusMillis(10L);
+		final IssEventInfo eventInfo = mock(IssEventInfo.class);
+		final TransactionRecord record = mock(TransactionRecord.class);
+		given(eventInfo.status()).willReturn(NO_KNOWN_ISS);
+
+		given(ctx.consensusTimeOfLastHandledTxn()).willReturn(then);
+		given(ctx.addressBook().getAddress(666).getStake()).willReturn(1L);
+		given(ctx.issEventInfo()).willReturn(eventInfo);
+		given(txnCtx.consensusTime()).willReturn(now);
+		given(txnBody.hasContractCreateInstance()).willReturn(true);
+		given(txnBody.getContractCreateInstance()).willReturn(ContractCreateTransactionBody.newBuilder()
+				.setMemo("This is a very long memo because it contains more than 100 characters, " +
+						"which is greater than it is expected")
+				.setFileID(FileID.newBuilder().build())
+				.setAutoRenewPeriod(Duration.newBuilder().setSeconds(10).build())
+				.build());
+
+		given(contracts.getFailureTransactionRecord(any(), any(), any())).willReturn(record);
+
+		// when:
+		subject.incorporateConsensusTxn(platformTxn, now, 666);
+
+		// then:
+		verify(contracts).getFailureTransactionRecord(txnBody, now, MEMO_TOO_LONG);
+	}
+
+	@Test
+	@DisplayName("creates a contract with small memo size")
+	public void contractCreateInstanceIsCreated() {
+		// setup:
+		final byte[] contractByteCode = new byte[] { 100 };
+		final SequenceNumber sequenceNumber = new SequenceNumber();
+		final Instant now = Instant.now();
+		final Instant then = now.minusMillis(10L);
+		final IssEventInfo eventInfo = mock(IssEventInfo.class);
+		final TransactionRecord record = mock(TransactionRecord.class);
+		given(eventInfo.status()).willReturn(NO_KNOWN_ISS);
+
+		given(ctx.consensusTimeOfLastHandledTxn()).willReturn(then);
+		given(ctx.addressBook().getAddress(666).getStake()).willReturn(1L);
+		given(ctx.issEventInfo()).willReturn(eventInfo);
+		given(ctx.seqNo()).willReturn(sequenceNumber);
+
+		given(txnCtx.consensusTime()).willReturn(now);
+		given(txnBody.hasContractCreateInstance()).willReturn(true);
+		given(txnBody.getContractCreateInstance()).willReturn(ContractCreateTransactionBody.newBuilder()
+				.setMemo("This is a very small memo")
+				.setFileID(FileID.newBuilder().build())
+				.setAutoRenewPeriod(Duration.newBuilder().setSeconds(10).build())
+				.build());
+		given(hfs.cat(any())).willReturn(contractByteCode);
+
+
+		// when:
+		subject.incorporateConsensusTxn(platformTxn, now, 666);
+
+		// then:
+		verify(contracts).createContract(txnBody, now, contractByteCode, sequenceNumber);
+	}
+
+	@Test
+	@DisplayName("creates a contract with no memo")
+	public void contractCreateInstanceIsCreatedNoMemo() {
+		// setup:
+		final byte[] contractByteCode = new byte[] { 100 };
+		final SequenceNumber sequenceNumber = new SequenceNumber();
+		final Instant now = Instant.now();
+		final Instant then = now.minusMillis(10L);
+		final IssEventInfo eventInfo = mock(IssEventInfo.class);
+		final TransactionRecord record = mock(TransactionRecord.class);
+		given(eventInfo.status()).willReturn(NO_KNOWN_ISS);
+
+		given(ctx.consensusTimeOfLastHandledTxn()).willReturn(then);
+		given(ctx.addressBook().getAddress(666).getStake()).willReturn(1L);
+		given(ctx.issEventInfo()).willReturn(eventInfo);
+		given(ctx.seqNo()).willReturn(sequenceNumber);
+
+		given(txnCtx.consensusTime()).willReturn(now);
+		given(txnBody.hasContractCreateInstance()).willReturn(true);
+		given(txnBody.getContractCreateInstance()).willReturn(ContractCreateTransactionBody.newBuilder()
+				.setFileID(FileID.newBuilder().build())
+				.setAutoRenewPeriod(Duration.newBuilder().setSeconds(10).build())
+				.build());
+		given(hfs.cat(any())).willReturn(contractByteCode);
+
+
+		// when:
+		subject.incorporateConsensusTxn(platformTxn, now, 666);
+
+		// then:
+		verify(contracts).createContract(txnBody, now, contractByteCode, sequenceNumber);
+	}
+
+	@Test
+	@DisplayName("incorporateConsensusTxn assigns a failure due to memo size for ContractUpdateInstance")
+	public void shortCircuitsOnMemoSizeForContractUpdate() {
+		// setup:
+		final Instant now = Instant.now();
+		final Instant then = now.minusMillis(10L);
+		final IssEventInfo eventInfo = mock(IssEventInfo.class);
+		final TransactionRecord record = mock(TransactionRecord.class);
+		given(eventInfo.status()).willReturn(NO_KNOWN_ISS);
+
+		given(ctx.consensusTimeOfLastHandledTxn()).willReturn(then);
+		given(ctx.addressBook().getAddress(666).getStake()).willReturn(1L);
+		given(ctx.issEventInfo()).willReturn(eventInfo);
+		given(txnCtx.consensusTime()).willReturn(now);
+		given(txnBody.hasContractUpdateInstance()).willReturn(true);
+		given(txnBody.getContractUpdateInstance()).willReturn(ContractUpdateTransactionBody.newBuilder()
+				.setMemo("This is a very long memo because it contains more than 100 characters, " +
+						"which is greater than it is expected")
+				.setFileID(FileID.newBuilder().build())
+				.setAutoRenewPeriod(Duration.newBuilder().setSeconds(10).build())
+				.build());
+
+		given(contracts.getFailureTransactionRecord(any(), any(), any())).willReturn(record);
+
+		// when:
+		subject.incorporateConsensusTxn(platformTxn, now, 666);
+
+		// then:
+		verify(contracts).getFailureTransactionRecord(txnBody, now, MEMO_TOO_LONG);
+	}
+
+	@Test
+	@DisplayName("ContractUpdateInstance is updated in contracts")
+	public void contractUpdateInstanceIsUpdate() {
+		// setup:
+		final Instant now = Instant.now();
+		final Instant then = now.minusMillis(10L);
+		final IssEventInfo eventInfo = mock(IssEventInfo.class);
+		final TransactionRecord record = mock(TransactionRecord.class);
+		given(eventInfo.status()).willReturn(NO_KNOWN_ISS);
+
+		given(ctx.consensusTimeOfLastHandledTxn()).willReturn(then);
+		given(ctx.addressBook().getAddress(666).getStake()).willReturn(1L);
+		given(ctx.issEventInfo()).willReturn(eventInfo);
+		given(txnCtx.consensusTime()).willReturn(now);
+		given(txnBody.hasContractUpdateInstance()).willReturn(true);
+		given(txnBody.getContractUpdateInstance()).willReturn(ContractUpdateTransactionBody.newBuilder()
+				.setMemo("This is a very small memo")
+				.setFileID(FileID.newBuilder().build())
+				.setAutoRenewPeriod(Duration.newBuilder().setSeconds(10).build())
+				.build());
+
+
+		// when:
+		subject.incorporateConsensusTxn(platformTxn, now, 666);
+
+		// then:
+		verify(contracts).updateContract(txnBody, now);
+	}
+
+}