From c19c0fbbf3cf22c4d9cae4a62facd6252244e09f Mon Sep 17 00:00:00 2001 From: DeividasK Date: Mon, 5 Aug 2024 18:00:18 +0300 Subject: [PATCH] Only allow retrying failed transmissions --- contracts/.gas-snapshot | 112 ++++++++++++++++++ .../src/v0.8/keystone/KeystoneForwarder.sol | 11 +- .../src/v0.8/keystone/interfaces/IRouter.sol | 19 ++- .../test/KeystoneForwarder_ReportTest.t.sol | 20 +++- 4 files changed, 153 insertions(+), 9 deletions(-) create mode 100644 contracts/.gas-snapshot diff --git a/contracts/.gas-snapshot b/contracts/.gas-snapshot new file mode 100644 index 00000000000..3a0354d539c --- /dev/null +++ b/contracts/.gas-snapshot @@ -0,0 +1,112 @@ +CapabilitiesRegistry_AddCapabilitiesTest:test_AddCapability_NoConfigurationContract() (gas: 154832) +CapabilitiesRegistry_AddCapabilitiesTest:test_AddCapability_WithConfiguration() (gas: 178813) +CapabilitiesRegistry_AddCapabilitiesTest:test_RevertWhen_CalledByNonAdmin() (gas: 24723) +CapabilitiesRegistry_AddCapabilitiesTest:test_RevertWhen_CapabilityExists() (gas: 145703) +CapabilitiesRegistry_AddCapabilitiesTest:test_RevertWhen_ConfigurationContractDoesNotMatchInterface() (gas: 94606) +CapabilitiesRegistry_AddCapabilitiesTest:test_RevertWhen_ConfigurationContractNotDeployed() (gas: 92961) +CapabilitiesRegistry_AddDONTest:test_AddDON() (gas: 372302) +CapabilitiesRegistry_AddDONTest:test_RevertWhen_CalledByNonAdmin() (gas: 19273) +CapabilitiesRegistry_AddDONTest:test_RevertWhen_CapabilityDoesNotExist() (gas: 169752) +CapabilitiesRegistry_AddDONTest:test_RevertWhen_DeprecatedCapabilityAdded() (gas: 239789) +CapabilitiesRegistry_AddDONTest:test_RevertWhen_DuplicateCapabilityAdded() (gas: 249596) +CapabilitiesRegistry_AddDONTest:test_RevertWhen_DuplicateNodeAdded() (gas: 116890) +CapabilitiesRegistry_AddDONTest:test_RevertWhen_FaultToleranceIsZero() (gas: 43358) +CapabilitiesRegistry_AddDONTest:test_RevertWhen_NodeAlreadyBelongsToWorkflowDON() (gas: 343924) +CapabilitiesRegistry_AddDONTest:test_RevertWhen_NodeDoesNotSupportCapability() (gas: 180150) +CapabilitiesRegistry_AddNodeOperatorsTest:test_AddNodeOperators() (gas: 184135) +CapabilitiesRegistry_AddNodeOperatorsTest:test_RevertWhen_CalledByNonAdmin() (gas: 17602) +CapabilitiesRegistry_AddNodeOperatorsTest:test_RevertWhen_NodeOperatorAdminAddressZero() (gas: 18498) +CapabilitiesRegistry_AddNodesTest:test_AddsNodeParams() (gas: 358448) +CapabilitiesRegistry_AddNodesTest:test_OwnerCanAddNodes() (gas: 358414) +CapabilitiesRegistry_AddNodesTest:test_RevertWhen_AddingDuplicateP2PId() (gas: 301229) +CapabilitiesRegistry_AddNodesTest:test_RevertWhen_AddingNodeWithInvalidCapability() (gas: 55174) +CapabilitiesRegistry_AddNodesTest:test_RevertWhen_AddingNodeWithInvalidNodeOperator() (gas: 24895) +CapabilitiesRegistry_AddNodesTest:test_RevertWhen_AddingNodeWithoutCapabilities() (gas: 27669) +CapabilitiesRegistry_AddNodesTest:test_RevertWhen_CalledByNonNodeOperatorAdminAndNonOwner() (gas: 25108) +CapabilitiesRegistry_AddNodesTest:test_RevertWhen_P2PIDEmpty() (gas: 27408) +CapabilitiesRegistry_AddNodesTest:test_RevertWhen_SignerAddressEmpty() (gas: 27047) +CapabilitiesRegistry_AddNodesTest:test_RevertWhen_SignerAddressNotUnique() (gas: 309679) +CapabilitiesRegistry_DeprecateCapabilitiesTest:test_DeprecatesCapability() (gas: 89807) +CapabilitiesRegistry_DeprecateCapabilitiesTest:test_EmitsEvent() (gas: 89935) +CapabilitiesRegistry_DeprecateCapabilitiesTest:test_RevertWhen_CalledByNonAdmin() (gas: 22944) +CapabilitiesRegistry_DeprecateCapabilitiesTest:test_RevertWhen_CapabilityDoesNotExist() (gas: 16231) +CapabilitiesRegistry_DeprecateCapabilitiesTest:test_RevertWhen_CapabilityIsDeprecated() (gas: 91264) +CapabilitiesRegistry_GetCapabilitiesTest:test_ReturnsCapabilities() (gas: 135553) +CapabilitiesRegistry_GetDONsTest:test_CorrectlyFetchesDONs() (gas: 65468) +CapabilitiesRegistry_GetDONsTest:test_DoesNotIncludeRemovedDONs() (gas: 64924) +CapabilitiesRegistry_GetHashedCapabilityTest:test_CorrectlyGeneratesHashedCapabilityId() (gas: 11428) +CapabilitiesRegistry_GetHashedCapabilityTest:test_DoesNotCauseIncorrectClashes() (gas: 13087) +CapabilitiesRegistry_GetNodeOperatorsTest:test_CorrectlyFetchesNodeOperators() (gas: 36407) +CapabilitiesRegistry_GetNodeOperatorsTest:test_DoesNotIncludeRemovedNodeOperators() (gas: 38692) +CapabilitiesRegistry_GetNodesTest:test_CorrectlyFetchesNodes() (gas: 65288) +CapabilitiesRegistry_GetNodesTest:test_DoesNotIncludeRemovedNodes() (gas: 73533) +CapabilitiesRegistry_RemoveDONsTest:test_RemovesDON() (gas: 54761) +CapabilitiesRegistry_RemoveDONsTest:test_RevertWhen_CalledByNonAdmin() (gas: 15647) +CapabilitiesRegistry_RemoveDONsTest:test_RevertWhen_DONDoesNotExist() (gas: 16550) +CapabilitiesRegistry_RemoveNodeOperatorsTest:test_RemovesNodeOperator() (gas: 36122) +CapabilitiesRegistry_RemoveNodeOperatorsTest:test_RevertWhen_CalledByNonOwner() (gas: 15816) +CapabilitiesRegistry_RemoveNodesTest:test_CanAddNodeWithSameSignerAddressAfterRemoving() (gas: 115151) +CapabilitiesRegistry_RemoveNodesTest:test_CanRemoveWhenDONDeleted() (gas: 287716) +CapabilitiesRegistry_RemoveNodesTest:test_CanRemoveWhenNodeNoLongerPartOfDON() (gas: 561023) +CapabilitiesRegistry_RemoveNodesTest:test_OwnerCanRemoveNodes() (gas: 73376) +CapabilitiesRegistry_RemoveNodesTest:test_RemovesNode() (gas: 75211) +CapabilitiesRegistry_RemoveNodesTest:test_RevertWhen_CalledByNonNodeOperatorAdminAndNonOwner() (gas: 25053) +CapabilitiesRegistry_RemoveNodesTest:test_RevertWhen_NodeDoesNotExist() (gas: 18418) +CapabilitiesRegistry_RemoveNodesTest:test_RevertWhen_NodePartOfCapabilitiesDON() (gas: 385369) +CapabilitiesRegistry_RemoveNodesTest:test_RevertWhen_P2PIDEmpty() (gas: 18408) +CapabilitiesRegistry_TypeAndVersionTest:test_TypeAndVersion() (gas: 9796) +CapabilitiesRegistry_UpdateDONTest:test_RevertWhen_CalledByNonAdmin() (gas: 19415) +CapabilitiesRegistry_UpdateDONTest:test_RevertWhen_CapabilityDoesNotExist() (gas: 152914) +CapabilitiesRegistry_UpdateDONTest:test_RevertWhen_DONDoesNotExist() (gas: 17835) +CapabilitiesRegistry_UpdateDONTest:test_RevertWhen_DeprecatedCapabilityAdded() (gas: 222996) +CapabilitiesRegistry_UpdateDONTest:test_RevertWhen_DuplicateCapabilityAdded() (gas: 232804) +CapabilitiesRegistry_UpdateDONTest:test_RevertWhen_DuplicateNodeAdded() (gas: 107643) +CapabilitiesRegistry_UpdateDONTest:test_RevertWhen_NodeDoesNotSupportCapability() (gas: 163357) +CapabilitiesRegistry_UpdateDONTest:test_UpdatesDON() (gas: 371909) +CapabilitiesRegistry_UpdateNodeOperatorTest:test_RevertWhen_CalledByNonAdminAndNonOwner() (gas: 20728) +CapabilitiesRegistry_UpdateNodeOperatorTest:test_RevertWhen_NodeOperatorAdminIsZeroAddress() (gas: 20052) +CapabilitiesRegistry_UpdateNodeOperatorTest:test_RevertWhen_NodeOperatorDoesNotExist() (gas: 19790) +CapabilitiesRegistry_UpdateNodeOperatorTest:test_RevertWhen_NodeOperatorIdAndParamLengthsMismatch() (gas: 15430) +CapabilitiesRegistry_UpdateNodeOperatorTest:test_UpdatesNodeOperator() (gas: 37034) +CapabilitiesRegistry_UpdateNodesTest:test_CanUpdateParamsIfNodeSignerAddressNoLongerUsed() (gas: 256371) +CapabilitiesRegistry_UpdateNodesTest:test_OwnerCanUpdateNodes() (gas: 162166) +CapabilitiesRegistry_UpdateNodesTest:test_RevertWhen_AddingNodeWithInvalidCapability() (gas: 35873) +CapabilitiesRegistry_UpdateNodesTest:test_RevertWhen_CalledByAnotherNodeOperatorAdmin() (gas: 29200) +CapabilitiesRegistry_UpdateNodesTest:test_RevertWhen_CalledByNonNodeOperatorAdminAndNonOwner() (gas: 29377) +CapabilitiesRegistry_UpdateNodesTest:test_RevertWhen_NodeDoesNotExist() (gas: 29199) +CapabilitiesRegistry_UpdateNodesTest:test_RevertWhen_NodeSignerAlreadyAssignedToAnotherNode() (gas: 31326) +CapabilitiesRegistry_UpdateNodesTest:test_RevertWhen_P2PIDEmpty() (gas: 29165) +CapabilitiesRegistry_UpdateNodesTest:test_RevertWhen_RemovingCapabilityRequiredByCapabilityDON() (gas: 470910) +CapabilitiesRegistry_UpdateNodesTest:test_RevertWhen_RemovingCapabilityRequiredByWorkflowDON() (gas: 341191) +CapabilitiesRegistry_UpdateNodesTest:test_RevertWhen_SignerAddressEmpty() (gas: 29058) +CapabilitiesRegistry_UpdateNodesTest:test_RevertWhen_UpdatingNodeWithoutCapabilities() (gas: 27587) +CapabilitiesRegistry_UpdateNodesTest:test_UpdatesNodeParams() (gas: 162220) +KeystoneForwarder_ReportTest:test_Report_ConfigVersion() (gas: 2002057) +KeystoneForwarder_ReportTest:test_Report_FailedDeliveryWhenReceiverInterfaceNotSupported() (gas: 128934) +KeystoneForwarder_ReportTest:test_Report_FailedDeliveryWhenReceiverNotContract() (gas: 130621) +KeystoneForwarder_ReportTest:test_Report_SuccessfulDelivery() (gas: 359123) +KeystoneForwarder_ReportTest:test_Report_SuccessfulRetryWithMoreGas() (gas: 423982) +KeystoneForwarder_ReportTest:test_RevertWhen_AnySignatureIsInvalid() (gas: 86348) +KeystoneForwarder_ReportTest:test_RevertWhen_AnySignerIsInvalid() (gas: 118486) +KeystoneForwarder_ReportTest:test_RevertWhen_ReportHasDuplicateSignatures() (gas: 94516) +KeystoneForwarder_ReportTest:test_RevertWhen_ReportHasIncorrectDON() (gas: 75930) +KeystoneForwarder_ReportTest:test_RevertWhen_ReportHasInexistentConfigVersion() (gas: 76320) +KeystoneForwarder_ReportTest:test_RevertWhen_ReportIsMalformed() (gas: 45585) +KeystoneForwarder_ReportTest:test_RevertWhen_RetryingInvalidContractTransmission() (gas: 143354) +KeystoneForwarder_ReportTest:test_RevertWhen_RetryingSuccessfulTransmission() (gas: 353272) +KeystoneForwarder_ReportTest:test_RevertWhen_TooFewSignatures() (gas: 55292) +KeystoneForwarder_ReportTest:test_RevertWhen_TooManySignatures() (gas: 56050) +KeystoneForwarder_SetConfigTest:test_RevertWhen_ExcessSigners() (gas: 20184) +KeystoneForwarder_SetConfigTest:test_RevertWhen_FaultToleranceIsZero() (gas: 88057) +KeystoneForwarder_SetConfigTest:test_RevertWhen_InsufficientSigners() (gas: 14533) +KeystoneForwarder_SetConfigTest:test_RevertWhen_NotOwner() (gas: 88766) +KeystoneForwarder_SetConfigTest:test_RevertWhen_ProvidingDuplicateSigners() (gas: 114570) +KeystoneForwarder_SetConfigTest:test_RevertWhen_ProvidingZeroAddressSigner() (gas: 114225) +KeystoneForwarder_SetConfigTest:test_SetConfig_FirstTime() (gas: 1540541) +KeystoneForwarder_SetConfigTest:test_SetConfig_WhenSignersAreRemoved() (gas: 1535211) +KeystoneForwarder_TypeAndVersionTest:test_TypeAndVersion() (gas: 9641) +KeystoneRouter_SetConfigTest:test_AddForwarder_RevertWhen_NotOwner() (gas: 10978) +KeystoneRouter_SetConfigTest:test_RemoveForwarder_RevertWhen_NotOwner() (gas: 10923) +KeystoneRouter_SetConfigTest:test_RemoveForwarder_Success() (gas: 17599) +KeystoneRouter_SetConfigTest:test_Route_RevertWhen_UnauthorizedForwarder() (gas: 18552) +KeystoneRouter_SetConfigTest:test_Route_Success() (gas: 76407) \ No newline at end of file diff --git a/contracts/src/v0.8/keystone/KeystoneForwarder.sol b/contracts/src/v0.8/keystone/KeystoneForwarder.sol index ec372876a63..719a34f6a1e 100644 --- a/contracts/src/v0.8/keystone/KeystoneForwarder.sol +++ b/contracts/src/v0.8/keystone/KeystoneForwarder.sol @@ -122,12 +122,15 @@ contract KeystoneForwarder is OwnerIsCreator, ITypeAndVersion, IRouter { if (!s_forwarders[msg.sender]) revert UnauthorizedForwarder(); TransmissionInfo memory transmission = s_transmissions[transmissionId]; - uint256 gasLeft = gasleft(); - if (transmission.transmitter != address(0) && transmission.gasLimit >= gasLeft) - revert AlreadyAttempted(transmissionId); + if (transmission.success || transmission.invalidReceiver) revert AlreadyAttempted(transmissionId); s_transmissions[transmissionId].transmitter = transmitter; - s_transmissions[transmissionId].gasLimit = uint88(gasLeft); + s_transmissions[transmissionId].gasLimit = uint80(gasleft()); + + if (receiver.code.length == 0) { + s_transmissions[transmissionId].invalidReceiver = true; + return false; + } bool success; bytes memory payload = abi.encodeCall(IReceiver.onReport, (metadata, validatedReport)); diff --git a/contracts/src/v0.8/keystone/interfaces/IRouter.sol b/contracts/src/v0.8/keystone/interfaces/IRouter.sol index ec10c51b1e8..1aa4ae71e98 100644 --- a/contracts/src/v0.8/keystone/interfaces/IRouter.sol +++ b/contracts/src/v0.8/keystone/interfaces/IRouter.sol @@ -15,18 +15,35 @@ interface IRouter { enum TransmissionState { NOT_ATTEMPTED, SUCCEEDED, + INVALID_RECEIVER, FAILED } struct TransmissionInfo { address transmitter; + // This is true if the receiver is not a contract or does not implement the + // `IReceiver` interface. + bool invalidReceiver; + // Whether the transmission attempt was successful. If `false`, the + // transmission can be retried with an increased gas limit. bool success; // The amount of gas allocated for the `IReceiver.onReport` call. uint88 // allows storing gas for known EVM block gas limits. // Ensures that the minimum gas requested by the user is available during // the transmission attempt. If the transmission fails (indicated by a // `false` success state), it can be retried with an increased gas limit. - uint88 gasLimit; + uint80 gasLimit; + } + + struct Transmission { + address transmitter; + TransmissionState state; + // The amount of gas allocated for the `IReceiver.onReport` call. uint88 + // allows storing gas for known EVM block gas limits. + // Ensures that the minimum gas requested by the user is available during + // the transmission attempt. If the transmission fails (indicated by a + // `false` success state), it can be retried with an increased gas limit. + uint80 gasLimit; } function addForwarder(address forwarder) external; diff --git a/contracts/src/v0.8/keystone/test/KeystoneForwarder_ReportTest.t.sol b/contracts/src/v0.8/keystone/test/KeystoneForwarder_ReportTest.t.sol index 8bd8e7f4122..bfa33162b4c 100644 --- a/contracts/src/v0.8/keystone/test/KeystoneForwarder_ReportTest.t.sol +++ b/contracts/src/v0.8/keystone/test/KeystoneForwarder_ReportTest.t.sol @@ -141,12 +141,24 @@ contract KeystoneForwarder_ReportTest is BaseTest { s_forwarder.report(address(s_receiver), report, reportContext, signatures); } - function test_RevertWhen_AlreadyAttempted() public { - s_forwarder.report(address(s_receiver), report, reportContext, signatures); + function test_RevertWhen_RetryingSuccessfulTransmission() public { + s_forwarder.report{gas: 400_000}(address(s_receiver), report, reportContext, signatures); bytes32 transmissionId = s_forwarder.getTransmissionId(address(s_receiver), executionId, reportId); vm.expectRevert(abi.encodeWithSelector(IRouter.AlreadyAttempted.selector, transmissionId)); - s_forwarder.report(address(s_receiver), report, reportContext, signatures); + // Retyring with more gas + s_forwarder.report{gas: 450_000}(address(s_receiver), report, reportContext, signatures); + } + + function test_RevertWhen_RetryingInvalidContractTransmission() public { + // Receiver is not a contract + address receiver = address(404); + s_forwarder.report{gas: 400_000}(receiver, report, reportContext, signatures); + + bytes32 transmissionId = s_forwarder.getTransmissionId(receiver, executionId, reportId); + vm.expectRevert(abi.encodeWithSelector(IRouter.AlreadyAttempted.selector, transmissionId)); + // Retyring with more gas + s_forwarder.report{gas: 450_000}(receiver, report, reportContext, signatures); } function test_Report_SuccessfulDelivery() public { @@ -178,7 +190,7 @@ contract KeystoneForwarder_ReportTest is BaseTest { // TODO: Add error for insufficient gas - function test_Report_SuccessfulRetryWithGas() public { + function test_Report_SuccessfulRetryWithMoreGas() public { s_forwarder.report{gas: 150_000}(address(s_receiver), report, reportContext, signatures); // Expect to fail with the receiver running out of gas