From 02ab7af6f4d87c5f3ae8ab9be3b550ccaeff974b Mon Sep 17 00:00:00 2001 From: eutopian Date: Mon, 9 Dec 2024 17:59:51 -0500 Subject: [PATCH] ensure that workflow id is unique --- contracts/gas-snapshots/workflow.gas-snapshot | 28 +++++------ .../v0.8/workflow/dev/WorkflowRegistry.sol | 47 +++++++++++++++---- .../WorkflowRegistry.registerWorkflow.t.sol | 29 ++++++++++++ .../WorkflowRegistry.registerWorkflow.tree | 2 + .../WorkflowRegistry.updateWorkflow.t.sol | 26 ++++++++++ .../WorkflowRegistry.updateWorkflow.tree | 2 + 6 files changed, 112 insertions(+), 22 deletions(-) diff --git a/contracts/gas-snapshots/workflow.gas-snapshot b/contracts/gas-snapshots/workflow.gas-snapshot index 9195c401ef3..3a1c3adfe27 100644 --- a/contracts/gas-snapshots/workflow.gas-snapshot +++ b/contracts/gas-snapshots/workflow.gas-snapshot @@ -17,9 +17,9 @@ WorkflowRegistryManager_getVersion:test_WhenVersionNumberIsRegistered() (gas: 28 WorkflowRegistryManager_getVersionNumberByContractAddressAndChainID:test_WhenAVersionIsRegisteredForTheContractAddressAndChainIDCombination() (gas: 285022) WorkflowRegistryManager_getVersionNumberByContractAddressAndChainID:test_WhenNoVersionIsRegisteredForTheContractAddressAndChainIDCombination() (gas: 286634) WorkflowRegistryManager_getVersionNumberByContractAddressAndChainID:test_WhenTheContractAddressIsInvalid() (gas: 284604) -WorkflowRegistry_activateWorkflow:test_WhenTheCallerIsAnAuthorizedAddress() (gas: 495029) -WorkflowRegistry_deleteWorkflow:test_WhenTheCallerIsAnAuthorizedAddress_AndTheDonIDIsAllowed() (gas: 403945) -WorkflowRegistry_deleteWorkflow:test_WhenTheCallerIsAnAuthorizedAddress_AndTheDonIDIsNotAllowed() (gas: 421748) +WorkflowRegistry_activateWorkflow:test_WhenTheCallerIsAnAuthorizedAddress() (gas: 517416) +WorkflowRegistry_deleteWorkflow:test_WhenTheCallerIsAnAuthorizedAddress_AndTheDonIDIsAllowed() (gas: 422167) +WorkflowRegistry_deleteWorkflow:test_WhenTheCallerIsAnAuthorizedAddress_AndTheDonIDIsNotAllowed() (gas: 439969) WorkflowRegistry_getAllAllowedDONs:test_WhenTheRegistryIsLocked() (gas: 47473) WorkflowRegistry_getAllAllowedDONs:test_WhenTheSetOfAllowedDONsIsEmpty() (gas: 25780) WorkflowRegistry_getAllAllowedDONs:test_WhenThereAreMultipleAllowedDONs() (gas: 75437) @@ -28,9 +28,9 @@ WorkflowRegistry_getAllAuthorizedAddresses:test_WhenTheRegistryIsLocked() (gas: WorkflowRegistry_getAllAuthorizedAddresses:test_WhenTheSetOfAuthorizedAddressesIsEmpty() (gas: 26152) WorkflowRegistry_getAllAuthorizedAddresses:test_WhenThereAreMultipleAuthorizedAddresses() (gas: 78270) WorkflowRegistry_getAllAuthorizedAddresses:test_WhenThereIsASingleAuthorizedAddress() (gas: 16832) -WorkflowRegistry_getWorkflowMetadata:test_WhenTheRegistryIsLocked() (gas: 519145) +WorkflowRegistry_getWorkflowMetadata:test_WhenTheRegistryIsLocked() (gas: 541532) WorkflowRegistry_getWorkflowMetadata:test_WhenTheWorkflowDoesNotExist() (gas: 17543) -WorkflowRegistry_getWorkflowMetadata:test_WhenTheWorkflowExistsWithTheOwnerAndName() (gas: 490001) +WorkflowRegistry_getWorkflowMetadata:test_WhenTheWorkflowExistsWithTheOwnerAndName() (gas: 512388) WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenLimitExceedsTotalWorkflows() (gas: 128146) WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenLimitIsEqualToTotalWorkflows() (gas: 128035) WorkflowRegistry_getWorkflowMetadataListByDON:test_WhenLimitIsLessThanTotalWorkflows() (gas: 90141) @@ -48,17 +48,17 @@ WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenStartIsGreaterThanOrEqu WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenTheOwnerHasNoWorkflows() (gas: 14006) WorkflowRegistry_getWorkflowMetadataListByOwner:test_WhenTheRegistryIsLocked() (gas: 165968) WorkflowRegistry_lockRegistry:test_WhenTheCallerIsTheContractOwner() (gas: 38758) -WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsAllowed_AndTheCallerIsAnAuthorizedAddress() (gas: 494993) -WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsAllowed_AndTheCallerIsAnUnauthorizedAddress() (gas: 502796) -WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsNotAllowed_AndTheCallerIsAnAuthorizedAddress() (gas: 502555) -WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsNotAllowed_AndTheCallerIsAnUnauthorizedAddress() (gas: 506966) -WorkflowRegistry_registerWorkflow:test_WhenTheWorkflowInputsAreAllValid() (gas: 549769) -WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsAnAuthorizedAddress_AndTheWorkflowIsInAnAllowedDON() (gas: 891242) -WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsAnAuthorizedAddress_AndTheWorkflowIsNotInAnAllowedDON() (gas: 488397) -WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsNotAnAuthorizedAddress() (gas: 486751) +WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsAllowed_AndTheCallerIsAnAuthorizedAddress() (gas: 517380) +WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsAllowed_AndTheCallerIsAnUnauthorizedAddress() (gas: 525183) +WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsNotAllowed_AndTheCallerIsAnAuthorizedAddress() (gas: 524942) +WorkflowRegistry_pauseWorkflow:test_WhenTheDonIDIsNotAllowed_AndTheCallerIsAnUnauthorizedAddress() (gas: 529353) +WorkflowRegistry_registerWorkflow:test_WhenTheWorkflowInputsAreAllValid() (gas: 572178) +WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsAnAuthorizedAddress_AndTheWorkflowIsInAnAllowedDON() (gas: 936016) +WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsAnAuthorizedAddress_AndTheWorkflowIsNotInAnAllowedDON() (gas: 510784) +WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsNotAnAuthorizedAddress() (gas: 509138) WorkflowRegistry_unlockRegistry:test_WhenTheCallerIsTheContractOwner() (gas: 30325) WorkflowRegistry_updateAllowedDONs:test_WhenTheBoolInputIsFalse() (gas: 29739) WorkflowRegistry_updateAllowedDONs:test_WhenTheBoolInputIsTrue() (gas: 170296) WorkflowRegistry_updateAuthorizedAddresses:test_WhenTheBoolInputIsFalse() (gas: 30278) WorkflowRegistry_updateAuthorizedAddresses:test_WhenTheBoolInputIsTrue() (gas: 175515) -WorkflowRegistry_updateWorkflow:test_WhenTheWorkflowInputsAreAllValid() (gas: 479601) \ No newline at end of file +WorkflowRegistry_updateWorkflow:test_WhenTheWorkflowInputsAreAllValid() (gas: 515676) diff --git a/contracts/src/v0.8/workflow/dev/WorkflowRegistry.sol b/contracts/src/v0.8/workflow/dev/WorkflowRegistry.sol index 0e6ae3450ac..b915965bb2b 100644 --- a/contracts/src/v0.8/workflow/dev/WorkflowRegistry.sol +++ b/contracts/src/v0.8/workflow/dev/WorkflowRegistry.sol @@ -43,6 +43,8 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { /// @dev Mapping to track workflows by secretsURL hash (owner + secretsURL). /// This is used to find all workflows that have the same secretsURL when a force secrets update event is requested. mapping(bytes32 secretsURLHash => EnumerableSet.Bytes32Set workflowKeys) private s_secretsHashToWorkflows; + /// @dev Keep track of all workflowIDs to ensure uniqueness. + mapping(bytes32 workflowID => bool inUse) private s_workflowIDs; /// @dev List of all authorized EOAs/contracts allowed to access this contract's state functions. All view functions are open access. EnumerableSet.AddressSet private s_authorizedAddresses; @@ -203,13 +205,15 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { ) external registryNotLocked { _validatePermissions(donID, msg.sender); _validateWorkflowName(bytes(workflowName).length); - _validateWorkflowMetadata(workflowID, bytes(binaryURL).length, bytes(configURL).length, bytes(secretsURL).length); + _validateWorkflowURLs(bytes(binaryURL).length, bytes(configURL).length, bytes(secretsURL).length); bytes32 workflowKey = computeHashKey(msg.sender, workflowName); if (s_workflows[workflowKey].owner != address(0)) { revert WorkflowAlreadyRegistered(); } + _requireUniqueWorkflowID(workflowID); + // Create new workflow entry s_workflows[workflowKey] = WorkflowMetadata({ workflowID: workflowID, @@ -272,7 +276,7 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { string calldata configURL, string calldata secretsURL ) external registryNotLocked { - _validateWorkflowMetadata(newWorkflowID, bytes(binaryURL).length, bytes(configURL).length, bytes(secretsURL).length); + _validateWorkflowURLs(bytes(binaryURL).length, bytes(configURL).length, bytes(secretsURL).length); WorkflowMetadata storage workflow = _getWorkflowFromStorage(msg.sender, workflowKey); @@ -295,6 +299,12 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { revert WorkflowContentNotUpdated(); } + // Ensure the new workflowID is unique + _requireUniqueWorkflowID(newWorkflowID); + + // Free the old workflowID + _releaseWorkflowID(currentWorkflowID); + // Update all fields that have changed and the relevant sets workflow.workflowID = newWorkflowID; if (!sameBinaryURL) { @@ -387,6 +397,9 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { revert AddressNotAuthorized(msg.sender); } + // Release the workflowID for reuse + _releaseWorkflowID(workflow.workflowID); + // Remove the workflow from the owner and DON mappings s_ownerWorkflowKeys[msg.sender].remove(workflowKey); s_donWorkflowKeys[workflow.donID].remove(workflowKey); @@ -508,6 +521,28 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { return workflow; } + /// @notice Ensures the given workflowID is unique and marks it as used. + /// @param workflowID The workflowID to validate and consume. + function _requireUniqueWorkflowID( + bytes32 workflowID + ) internal { + if (workflowID == bytes32(0)) revert InvalidWorkflowID(); + + if (s_workflowIDs[workflowID]) { + revert WorkflowIDAlreadyExists(); + } + + s_workflowIDs[workflowID] = true; + } + + /// @notice Frees up a previously used workflowID, allowing it to be reused. + /// @param workflowID The workflowID to release. + function _releaseWorkflowID( + bytes32 workflowID + ) internal { + s_workflowIDs[workflowID] = false; + } + // ================================================================ // | Workflow Queries | // ================================================================ @@ -636,16 +671,12 @@ contract WorkflowRegistry is Ownable2StepMsgSender, ITypeAndVersion { // | Validation | // ================================================================ - /// @dev Internal function to validate the metadata for a workflow. - /// @param workflowID The unique identifier for the workflow. - function _validateWorkflowMetadata( - bytes32 workflowID, + /// @dev Internal function to validate the urls for a workflow. + function _validateWorkflowURLs( uint256 binaryURLLength, uint256 configURLLength, uint256 secretsURLLength ) internal pure { - if (workflowID == bytes32(0)) revert InvalidWorkflowID(); - if (binaryURLLength > MAX_URL_LENGTH) { revert URLTooLong(binaryURLLength, MAX_URL_LENGTH); } diff --git a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.t.sol b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.t.sol index 426fbfcc502..859437196cd 100644 --- a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.t.sol +++ b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.t.sol @@ -138,6 +138,35 @@ contract WorkflowRegistry_registerWorkflow is WorkflowRegistrySetup { ); } + // whenTheCallerIsAnAuthorizedAddress whenTheRegistryIsNotLocked whenTheDonIDIsAllowed + function test_RevertWhen_TheWorkflowIDIsAlreadyInUsedByAnotherWorkflow() external { + vm.startPrank(s_authorizedAddress); + + // Register a valid workflow first + s_registry.registerWorkflow( + s_validWorkflowName, + s_validWorkflowID, + s_allowedDonID, + WorkflowRegistry.WorkflowStatus.ACTIVE, + s_validBinaryURL, + s_validConfigURL, + s_validSecretsURL + ); + + vm.expectRevert(WorkflowRegistry.WorkflowIDAlreadyExists.selector); + s_registry.registerWorkflow( + "ValidWorkflow2", + s_validWorkflowID, + s_allowedDonID, + WorkflowRegistry.WorkflowStatus.ACTIVE, + s_validBinaryURL, + s_validConfigURL, + s_validSecretsURL + ); + + vm.stopPrank(); + } + // whenTheCallerIsAnAuthorizedAddress whenTheRegistryIsNotLocked whenTheDonIDIsAllowed function test_RevertWhen_TheWorkflowNameIsAlreadyUsedByTheOwner() external { vm.startPrank(s_authorizedAddress); diff --git a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.tree b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.tree index 75cdf940575..eabbf58d464 100644 --- a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.tree +++ b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.registerWorkflow.tree @@ -18,6 +18,8 @@ WorkflowRegistry.registerWorkflow │ └── it should revert ├── when the workflowID is invalid │ └── it should revert + ├── when the workflowID is already in used by another workflow + │ └── it should revert ├── when the workflow name is already used by the owner │ └── it should revert └── when the workflow inputs are all valid diff --git a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.t.sol b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.t.sol index 5058512ba7b..4082874a91e 100644 --- a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.t.sol +++ b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.t.sol @@ -158,6 +158,32 @@ contract WorkflowRegistry_updateWorkflow is WorkflowRegistrySetup { s_registry.updateWorkflow(s_validWorkflowKey, bytes32(0), s_validBinaryURL, s_validConfigURL, s_newValidSecretsURL); } + // whenTheCallerIsAnAuthorizedAddress whenTheRegistryIsNotLocked whenTheDonIDIsAllowed whenTheCallerIsTheWorkflowOwner + function test_RevertWhen_TheWorkflowIDIsAlreadyInUsedByAnotherWorkflow() external { + // Register a workflow first + _registerValidWorkflow(); + + // Register another workflow with another workflow ID + vm.startPrank(s_authorizedAddress); + s_registry.registerWorkflow( + "ValidWorkflow2", + s_newValidWorkflowID, + s_allowedDonID, + WorkflowRegistry.WorkflowStatus.ACTIVE, + s_validBinaryURL, + s_validConfigURL, + s_validSecretsURL + ); + + // Update the workflow with a workflow ID that is already in use by another workflow. + vm.expectRevert(WorkflowRegistry.WorkflowIDAlreadyExists.selector); + s_registry.updateWorkflow( + s_validWorkflowKey, s_newValidWorkflowID, s_validBinaryURL, s_validConfigURL, s_newValidSecretsURL + ); + + vm.stopPrank(); + } + // whenTheCallerIsAnAuthorizedAddress whenTheRegistryIsNotLocked whenTheDonIDIsAllowed whenTheCallerIsTheWorkflowOwner function test_WhenTheWorkflowInputsAreAllValid() external { // Register a workflow first. diff --git a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.tree b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.tree index 0d4da7cb32e..9b8243a8672 100644 --- a/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.tree +++ b/contracts/src/v0.8/workflow/test/WorkflowRegistry/WorkflowRegistry.updateWorkflow.tree @@ -25,6 +25,8 @@ WorkflowRegistry.updateWorkflow │ └── it should revert ├── when the workflowID is invalid │ └── it should revert + ├── when the workflowID is already in used by another workflow + │ └── it should revert └── when the workflow inputs are all valid ├── it should update the existing workflow in s_workflows with the new values ├── it should emit {WorkflowUpdatedV1}