Skip to content

Fix scheduler DoS from create-vault type resolution#68

Draft
liobrasil wants to merge 1 commit intomainfrom
lionel/scheduler-dos-long-term-fix
Draft

Fix scheduler DoS from create-vault type resolution#68
liobrasil wants to merge 1 commit intomainfrom
lionel/scheduler-dos-long-term-fix

Conversation

@liobrasil
Copy link
Collaborator

Summary

  • addresses Navid's scheduler DoS report in Invalid Strategy Config Panicked Scheduler #67 (Invalid Strategy Config Panicked Scheduler)
  • replace scheduler-side dynamic type resolution for CREATE_YIELDVAULT requests with an immutable createVaultConfigId registry model
  • add an atomic Cadence transaction to register CREATE_YIELDVAULT configs on both Cadence and EVM in one operation
  • harden Cadence registration so partial/direct registration paths are not externally callable and Cadence mirrors pair uniqueness checks
  • update deploy scripts, workflows, tests, helper scripts, artifacts, and docs to match the new config-registry model and fresh-deploy seeding flow

Why

The old scheduler preprocessing path could panic while validating CREATE_YIELDVAULT requests when a user-supplied strategy identifier resolved to a valid-looking but incompatible Cadence type. That made the scheduler vulnerable to a DoS condition even though scheduler failure is supposed to be impossible.

This change removes non-total validation from the scheduler path. Scheduler preprocessing now only checks for registered create-vault config presence. Dynamic type resolution stays out of the scheduler and any bad config is isolated to registration time or worker execution.

Main Changes

  • introduce immutable createVaultConfigId handling for CREATE_YIELDVAULT requests in Solidity and Cadence
  • make the scheduler/worker bridge decode and use config IDs instead of raw type strings for create-vault processing
  • add register_create_yieldvault_config_everywhere.cdc to register the same config on Cadence and EVM atomically through the worker COA
  • remove the old Cadence-only create-vault registration transaction
  • make deploy and verification scripts seed initial create-vault config safely and idempotently
  • update worker/testnet/e2e helpers and GitHub workflows for the new admission behavior and config-ID flow
  • refresh docs and exported ABI artifact to reflect the new request shape and ops flow

Validation

  • forge test --match-contract FlowYieldVaultsRequestsTest
  • flow test cadence/tests/config_registration_test.cdc cadence/tests/access_control_test.cdc cadence/tests/error_handling_test.cdc cadence/tests/evm_bridge_lifecycle_test.cdc cadence/tests/validation_test.cdc
  • bash -n local/run_worker_tests.sh local/run_e2e_tests.sh local/run_admin_e2e_tests.sh local/testnet-e2e.sh local/deploy_full_stack.sh local/deploy_and_verify.sh

@claude
Copy link

claude bot commented Mar 11, 2026

PR Review: Fix scheduler DoS from create-vault type resolution

This PR cleanly addresses the DoS vector from issue 67 by moving CREATE_YIELDVAULT parameter validation from the scheduler runtime preprocessing path into an admin-gated config registry. The core design is sound and the implementation is thorough.

Security Fix Assessment

The replacement of dynamic CompositeType() / getSupportedStrategies() resolution in validateCreateYieldVaultParameters with a simple registry lookup in validateCreateYieldVaultConfig is correct. The scheduler can no longer panic on user-supplied type strings because those strings never reach it. All type validation is deferred to admin registration time, or as a last-resort guard, to the worker execution path.

The atomic two-step registration via registerCreateYieldVaultConfigEverywhere (Cadence-first, then EVM COA call) with Cadence transaction rollback on EVM failure is a good use of the cross-VM atomicity guarantee in CLAUDE.md.

Issues and Observations

1. enabled field exists on EVM but not on Cadence (asymmetry)

FlowYieldVaultsRequests.sol has CreateYieldVaultConfig.enabled and setCreateYieldVaultConfigEnabled(). The Cadence CreateYieldVaultConfig struct has no enabled field. Disabling a config on EVM correctly blocks new requests at admission; existing PENDING requests are not blocked since Cadence only checks config != nil. This is correct behavior, but a short doc comment clarifying the asymmetry would help future maintainers.

2. createYieldVaultConfigIdByPairHash mapping is public

The mapping exposes the keccak256(abi.encode(...)) hash scheme as an implicit API surface. If the hash function ever changes, any tooling reading the raw mapping would break silently. Consider making it private (keeping only the view function) or adding a NatSpec comment documenting the exact hash scheme.

3. Missing test: disabling a config does not affect already-PENDING requests

test_CreateYieldVault_RevertDisabledConfigId verifies admission is blocked after disabling, but there is no test confirming a PENDING request created before the disable can still be processed through startProcessingBatch to completeProcessing. This would close the coverage gap for the enabled disable/process invariant.

4. Breaking ABI changes are handled correctly

getRequest now returns RequestView instead of Request; getRequestUnpacked drops 2 return values and adds 1. The ABI artifact, FRONTEND_INTEGRATION.md, all shell scripts, and the Foundry deploy script are all updated consistently.

5. Gas improvement in bulk reads

Changing from Request memory req to Request storage req in getPendingRequestsUnpacked avoids copying the full struct to memory on each iteration. Good catch.

6. RequestCreated event backward compatibility preserved

_createRequest resolves vaultIdentifier and strategyIdentifier from the config registry before emitting RequestCreated, keeping the event signature unchanged.

7. deploy_and_verify.sh idempotency logic is correct

The registered_id != 0 check correctly treats the sentinel return of getCreateYieldVaultConfigId as not-found, consistent with configId >= 1 being enforced at registration.

Test Coverage

Area Status
Config registration + rollback on EVM failure config_registration_test.cdc
Direct Admin-only path forbidden testDirectCadenceOnlyConfigRegistrationIsForbidden
Direct Worker-only path forbidden testDirectEVMOnlyConfigRegistrationIsForbidden
New createYieldVault(configId) overload test_CreateYieldVault_WithConfigId
Unknown config ID revert test_CreateYieldVault_RevertUnknownConfigId
Disabled config revert at admission test_CreateYieldVault_RevertDisabledConfigId
Unregistered pair revert via legacy path test_CreateYieldVault_RevertUnregisteredPair
Bulk unpack returns config IDs test_GetPendingRequestsUnpacked_UsesConfigIds
getRequestUnpacked returns config ID test_GetRequestUnpacked_IncludesConfigId
PENDING requests unaffected by enabled=false Missing

Summary

The fix is correct and well-scoped. Main items to consider before merge:

  1. (Suggested) Add a comment or test clarifying that setCreateYieldVaultConfigEnabled(false) gates new requests only, not in-flight PENDING ones.
  2. (Optional) Decide whether createYieldVaultConfigIdByPairHash should be private or document its hash scheme in NatSpec.
  3. Everything else looks clean - access control, atomicity guarantees, ABI migration, and script updates are all handled correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant