From de61d29d5d01bb7fe168ef0cb09e3cf26dcf0427 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 12 May 2025 17:55:33 +0000 Subject: [PATCH 1/6] feat(pulse): add maximum deposit limit for permanent subscriptions Co-Authored-By: Tejas Badadare --- .../contracts/contracts/pulse/Scheduler.sol | 22 +++- .../contracts/pulse/SchedulerErrors.sol | 1 + .../contracts/pulse/SchedulerState.sol | 2 + .../contracts/forge-test/PulseScheduler.t.sol | 109 ++++++++++++++++++ 4 files changed, 133 insertions(+), 1 deletion(-) diff --git a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol index 4aa5e17f97..a0a46b19a8 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol @@ -42,6 +42,11 @@ abstract contract Scheduler is IScheduler, SchedulerState { revert InsufficientBalance(); } + // Check deposit limit for permanent subscriptions + if (subscriptionParams.isPermanent && msg.value > MAX_DEPOSIT_LIMIT) { + revert MaxDepositLimitExceeded(); + } + // Set subscription to active subscriptionParams.isActive = true; @@ -523,7 +528,22 @@ abstract contract Scheduler is IScheduler, SchedulerState { revert InactiveSubscription(); } - _state.subscriptionStatuses[subscriptionId].balanceInWei += msg.value; + SubscriptionParams storage params = _state.subscriptionParams[ + subscriptionId + ]; + SubscriptionStatus storage status = _state.subscriptionStatuses[ + subscriptionId + ]; + + // Check deposit limit for permanent subscriptions + if ( + params.isPermanent && + status.balanceInWei + msg.value > MAX_DEPOSIT_LIMIT + ) { + revert MaxDepositLimitExceeded(); + } + + status.balanceInWei += msg.value; } function withdrawFunds( diff --git a/target_chains/ethereum/contracts/contracts/pulse/SchedulerErrors.sol b/target_chains/ethereum/contracts/contracts/pulse/SchedulerErrors.sol index 899af3a840..0aaf0d0d6c 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/SchedulerErrors.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/SchedulerErrors.sol @@ -36,3 +36,4 @@ error DuplicateWhitelistAddress(address addr); // Payment errors error KeeperPaymentFailed(); +error MaxDepositLimitExceeded(); diff --git a/target_chains/ethereum/contracts/contracts/pulse/SchedulerState.sol b/target_chains/ethereum/contracts/contracts/pulse/SchedulerState.sol index e4c086348f..44c953b9c7 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/SchedulerState.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/SchedulerState.sol @@ -9,6 +9,8 @@ contract SchedulerState { uint8 public constant MAX_PRICE_IDS_PER_SUBSCRIPTION = 255; /// Maximum number of addresses in the reader whitelist uint8 public constant MAX_READER_WHITELIST_SIZE = 255; + /// Maximum deposit limit for permanent subscriptions in wei (100 ETH) + uint256 public constant MAX_DEPOSIT_LIMIT = 100 ether; /// Maximum time in the past (relative to current block timestamp) /// for which a price update timestamp is considered valid diff --git a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol index ee06ff171f..783d86d8ad 100644 --- a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol +++ b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol @@ -779,6 +779,115 @@ contract SchedulerTest is Test, SchedulerEvents, PulseSchedulerTestUtils { scheduler.updateSubscription(subscriptionId, params); } + function testPermanentSubscriptionDepositLimit() public { + // Test 1: Creating a permanent subscription with deposit exceeding MAX_DEPOSIT_LIMIT should fail + SchedulerState.SubscriptionParams + memory params = createDefaultSubscriptionParams(2, address(reader)); + params.isPermanent = true; + + uint256 maxDepositLimit = 100 ether; // Same as MAX_DEPOSIT_LIMIT in SchedulerState + uint256 excessiveDeposit = maxDepositLimit + 1 ether; + vm.deal(address(this), excessiveDeposit); + + vm.expectRevert( + abi.encodeWithSelector(MaxDepositLimitExceeded.selector) + ); + scheduler.createSubscription{value: excessiveDeposit}(params); + + // Test 2: Creating a permanent subscription with deposit within MAX_DEPOSIT_LIMIT should succeed + uint256 validDeposit = maxDepositLimit; + vm.deal(address(this), validDeposit); + + uint256 subscriptionId = scheduler.createSubscription{ + value: validDeposit + }(params); + + // Verify subscription was created correctly + ( + SchedulerState.SubscriptionParams memory storedParams, + SchedulerState.SubscriptionStatus memory status + ) = scheduler.getSubscription(subscriptionId); + + assertTrue( + storedParams.isPermanent, + "Subscription should be permanent" + ); + assertEq( + status.balanceInWei, + validDeposit, + "Balance should match deposit amount" + ); + + // Test 3: Adding funds to a permanent subscription that would exceed MAX_DEPOSIT_LIMIT should fail + uint256 additionalFunds = 1 wei; + vm.deal(address(this), additionalFunds); + + vm.expectRevert( + abi.encodeWithSelector(MaxDepositLimitExceeded.selector) + ); + scheduler.addFunds{value: additionalFunds}(subscriptionId); + + // Test 4: Adding funds to a permanent subscription within MAX_DEPOSIT_LIMIT should succeed + // First withdraw some funds to create space under the limit + uint256 withdrawAmount = 10 ether; + + // Create a non-permanent subscription to test partial funding + SchedulerState.SubscriptionParams + memory nonPermanentParams = createDefaultSubscriptionParams( + 2, + address(reader) + ); + uint256 minimumBalance = scheduler.getMinimumBalance( + uint8(nonPermanentParams.priceIds.length) + ); + vm.deal(address(this), minimumBalance); + + uint256 nonPermanentSubId = scheduler.createSubscription{ + value: minimumBalance + }(nonPermanentParams); + + // Add funds to the non-permanent subscription (should be within limit) + uint256 validAdditionalFunds = 5 ether; + vm.deal(address(this), validAdditionalFunds); + + scheduler.addFunds{value: validAdditionalFunds}(nonPermanentSubId); + + // Verify funds were added correctly + ( + , + SchedulerState.SubscriptionStatus memory nonPermanentStatus + ) = scheduler.getSubscription(nonPermanentSubId); + + assertEq( + nonPermanentStatus.balanceInWei, + minimumBalance + validAdditionalFunds, + "Balance should be increased by the funded amount" + ); + + // Test 5: Non-permanent subscriptions should not be subject to the deposit limit + uint256 largeDeposit = maxDepositLimit * 2; + vm.deal(address(this), largeDeposit); + + SchedulerState.SubscriptionParams + memory unlimitedParams = createDefaultSubscriptionParams( + 2, + address(reader) + ); + uint256 unlimitedSubId = scheduler.createSubscription{ + value: largeDeposit + }(unlimitedParams); + + // Verify subscription was created with the large deposit + (, SchedulerState.SubscriptionStatus memory unlimitedStatus) = scheduler + .getSubscription(unlimitedSubId); + + assertEq( + unlimitedStatus.balanceInWei, + largeDeposit, + "Non-permanent subscription should accept large deposits" + ); + } + function testAnyoneCanAddFunds() public { // Create a subscription uint256 subscriptionId = addTestSubscription( From c6295784e1329aa76c4176f467c0a6a7727957c5 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 12 May 2025 18:02:50 +0000 Subject: [PATCH 2/6] fix: remove unused variable in PulseScheduler.t.sol Co-Authored-By: Tejas Badadare --- .../ethereum/contracts/forge-test/PulseScheduler.t.sol | 3 --- 1 file changed, 3 deletions(-) diff --git a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol index 783d86d8ad..1adedd52e4 100644 --- a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol +++ b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol @@ -828,9 +828,6 @@ contract SchedulerTest is Test, SchedulerEvents, PulseSchedulerTestUtils { scheduler.addFunds{value: additionalFunds}(subscriptionId); // Test 4: Adding funds to a permanent subscription within MAX_DEPOSIT_LIMIT should succeed - // First withdraw some funds to create space under the limit - uint256 withdrawAmount = 10 ether; - // Create a non-permanent subscription to test partial funding SchedulerState.SubscriptionParams memory nonPermanentParams = createDefaultSubscriptionParams( From 0551da5cdb78e6b7544853b4d04711f22e66be39 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 12 May 2025 18:11:53 +0000 Subject: [PATCH 3/6] fix: update deposit limit check to only check incoming deposit amount Co-Authored-By: Tejas Badadare --- .../ethereum/contracts/contracts/pulse/Scheduler.sol | 5 +---- .../ethereum/contracts/forge-test/PulseScheduler.t.sol | 8 ++++---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol index a0a46b19a8..1d37af73f4 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol @@ -536,10 +536,7 @@ abstract contract Scheduler is IScheduler, SchedulerState { ]; // Check deposit limit for permanent subscriptions - if ( - params.isPermanent && - status.balanceInWei + msg.value > MAX_DEPOSIT_LIMIT - ) { + if (params.isPermanent && msg.value > MAX_DEPOSIT_LIMIT) { revert MaxDepositLimitExceeded(); } diff --git a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol index 1adedd52e4..10352ccda9 100644 --- a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol +++ b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol @@ -818,14 +818,14 @@ contract SchedulerTest is Test, SchedulerEvents, PulseSchedulerTestUtils { "Balance should match deposit amount" ); - // Test 3: Adding funds to a permanent subscription that would exceed MAX_DEPOSIT_LIMIT should fail - uint256 additionalFunds = 1 wei; - vm.deal(address(this), additionalFunds); + // Test 3: Adding funds to a permanent subscription with deposit exceeding MAX_DEPOSIT_LIMIT should fail + uint256 largeAdditionalFunds = maxDepositLimit + 1; + vm.deal(address(this), largeAdditionalFunds); vm.expectRevert( abi.encodeWithSelector(MaxDepositLimitExceeded.selector) ); - scheduler.addFunds{value: additionalFunds}(subscriptionId); + scheduler.addFunds{value: largeAdditionalFunds}(subscriptionId); // Test 4: Adding funds to a permanent subscription within MAX_DEPOSIT_LIMIT should succeed // Create a non-permanent subscription to test partial funding From 422dca809b88499c5a666e7c02c1f3850bf085ee Mon Sep 17 00:00:00 2001 From: Tejas Badadare <17058023+tejasbadadare@users.noreply.github.com> Date: Mon, 12 May 2025 11:19:18 -0700 Subject: [PATCH 4/6] fix: remove redundant access --- .../ethereum/contracts/contracts/pulse/Scheduler.sol | 9 +++++---- .../contracts/contracts/pulse/SchedulerState.sol | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol index 1d37af73f4..b5222bc55a 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol @@ -524,16 +524,17 @@ abstract contract Scheduler is IScheduler, SchedulerState { /// BALANCE MANAGEMENT function addFunds(uint256 subscriptionId) external payable override { - if (!_state.subscriptionParams[subscriptionId].isActive) { - revert InactiveSubscription(); - } - SubscriptionParams storage params = _state.subscriptionParams[ subscriptionId ]; SubscriptionStatus storage status = _state.subscriptionStatuses[ subscriptionId ]; + + if (!status.isActive) { + revert InactiveSubscription(); + } + // Check deposit limit for permanent subscriptions if (params.isPermanent && msg.value > MAX_DEPOSIT_LIMIT) { diff --git a/target_chains/ethereum/contracts/contracts/pulse/SchedulerState.sol b/target_chains/ethereum/contracts/contracts/pulse/SchedulerState.sol index 44c953b9c7..f4557cf297 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/SchedulerState.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/SchedulerState.sol @@ -9,7 +9,7 @@ contract SchedulerState { uint8 public constant MAX_PRICE_IDS_PER_SUBSCRIPTION = 255; /// Maximum number of addresses in the reader whitelist uint8 public constant MAX_READER_WHITELIST_SIZE = 255; - /// Maximum deposit limit for permanent subscriptions in wei (100 ETH) + /// Maximum deposit limit for permanent subscriptions in wei uint256 public constant MAX_DEPOSIT_LIMIT = 100 ether; /// Maximum time in the past (relative to current block timestamp) From b31a87c4dd4695d8dc7cb60ef09b6a8a42fbabd6 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 12 May 2025 19:02:07 +0000 Subject: [PATCH 5/6] fix: update isActive check in addFunds function Co-Authored-By: Tejas Badadare --- target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol index b5222bc55a..9b5a8e0533 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol @@ -531,11 +531,10 @@ abstract contract Scheduler is IScheduler, SchedulerState { subscriptionId ]; - if (!status.isActive) { + if (!params.isActive) { revert InactiveSubscription(); } - // Check deposit limit for permanent subscriptions if (params.isPermanent && msg.value > MAX_DEPOSIT_LIMIT) { revert MaxDepositLimitExceeded(); From 5c77ef37711580dd1bc9aa29c7e91b8ff4011896 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 12 May 2025 19:08:06 +0000 Subject: [PATCH 6/6] fix: format Scheduler.sol Co-Authored-By: Tejas Badadare --- target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol index 9b5a8e0533..38484d0458 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol @@ -530,7 +530,7 @@ abstract contract Scheduler is IScheduler, SchedulerState { SubscriptionStatus storage status = _state.subscriptionStatuses[ subscriptionId ]; - + if (!params.isActive) { revert InactiveSubscription(); }