Merge branch 'main' into fix/randomness-sender-bug-fix

najienka · najienka · commit de847b5d31c6 · 2025-10-02T11:19:40.000+01:00
diff --git a/package.json b/package.json
@@ -5,7 +5,7 @@
     "publishConfig": {
         "access": "public"
     },
-    "version": "0.0.7",
+    "version": "0.0.9",
     "directories": {
         "lib": "lib",
         "test": "test"
diff --git a/src/chainlink_compatible/ChainlinkVRFCoordinatorV2_5Adapter.sol b/src/chainlink_compatible/ChainlinkVRFCoordinatorV2_5Adapter.sol
@@ -36,6 +36,8 @@ contract ChainlinkVRFCoordinatorV2_5Adapter is
 
     event WrapperFulfillmentFailed(uint256 indexed requestId, address indexed consumer);
     event WrapperGasOverheadUpdated(uint32 newWrapperGasOverhead);
+    event SubscriptionOwnerTransferRequested(uint256 indexed subId, address from, address to);
+    event SubscriptionOwnerTransferred(uint256 indexed subId, address from, address to);
 
     /// @notice The contract responsible for providing randomness.
     /// @dev This is an immutable reference set at deployment.
@@ -50,6 +52,7 @@ contract ChainlinkVRFCoordinatorV2_5Adapter is
     uint256 public lastRequestId;
 
     mapping(uint256 => address) private subscriptionOwners;
+    mapping(uint256 => address) private pendingSubscriptionOwners;
     mapping(uint256 => Callback) /* requestID */ /* callback */ public s_callbacks;
 
     /// @notice Ensures that only the designated randomness sender can call the function.
@@ -241,22 +244,57 @@ contract ChainlinkVRFCoordinatorV2_5Adapter is
     /// @param subId - ID of the subscription
     /// @param to - Where to send the remaining native tokens to, e.g., Ether.
     function cancelSubscription(uint256 subId, address to) external override onlySubscriptionOwner(subId) {
-        randomnessSender.cancelSubscription(subId, to);
+        // Get the subscription balance before cancellation
+        (uint96 nativeBalance,,,) = randomnessSender.getSubscription(subId);
+
+        // Cancel the subscription - funds will come to this contract since it's the actual owner
+        randomnessSender.cancelSubscription(subId, address(this));
+
+        // Forward the funds to the intended recipient
+        if (nativeBalance > 0) {
+            _transferNative(to, nativeBalance);
+        }
+
+        // Clear the subscription owner mapping since subscription is cancelled
+        delete subscriptionOwners[subId];
+        delete pendingSubscriptionOwners[subId];
     }
 
     /// @notice Accept subscription owner transfer.
     /// @param subId - ID of the subscription
-    /// @dev will revert if original owner of subId has
-    /// not requested that msg.sender become the new owner.
-    function acceptSubscriptionOwnerTransfer(uint256 subId) external override onlySubscriptionOwner(subId) {
-        randomnessSender.acceptSubscriptionOwnerTransfer(subId);
+    /// @dev will accept ownership transfer if msg.sender is the pending owner.
+    function acceptSubscriptionOwnerTransfer(uint256 subId) external override {
+        address pendingOwner = pendingSubscriptionOwners[subId];
+        require(pendingOwner != address(0), "No pending ownership transfer");
+        require(msg.sender == pendingOwner, "Caller is not the pending owner");
+
+        address previousOwner = subscriptionOwners[subId];
+
+        // Update the subscription owner mapping to the new owner
+        subscriptionOwners[subId] = pendingOwner;
+
+        // Clear the pending transfer
+        delete pendingSubscriptionOwners[subId];
+
+        emit SubscriptionOwnerTransferred(subId, previousOwner, pendingOwner);
     }
 
     /// @notice Request subscription owner transfer.
+    /// @notice The onlySubscriptionOwner modifier is used to ensure proper access control is implemented for this function to restrict usage to authorized accounts only.
     /// @param subId - ID of the subscription
     /// @param newOwner - proposed new owner of the subscription
-    function requestSubscriptionOwnerTransfer(uint256 subId, address newOwner) external override {
-        randomnessSender.requestSubscriptionOwnerTransfer(subId, newOwner);
+    function requestSubscriptionOwnerTransfer(uint256 subId, address newOwner)
+        external
+        override
+        onlySubscriptionOwner(subId)
+    {
+        require(newOwner != address(0), "New owner cannot be zero address");
+        require(newOwner != subscriptionOwners[subId], "New owner is the same as current owner");
+
+        // Only update the pending owner in the adapter, don't call the underlying contract
+        pendingSubscriptionOwners[subId] = newOwner;
+
+        emit SubscriptionOwnerTransferRequested(subId, subscriptionOwners[subId], newOwner);
     }
 
     /// @notice Create a VRF subscription.
@@ -316,4 +354,28 @@ contract ChainlinkVRFCoordinatorV2_5Adapter is
     function fundSubscriptionWithNative(uint256 subId) external payable override {
         randomnessSender.fundSubscriptionWithNative{value: msg.value}(subId);
     }
+
+    /// @notice Get the pending owner for a subscription
+    /// @param subId - ID of the subscription
+    /// @return The address of the pending owner, or zero address if no transfer pending
+    function getPendingSubscriptionOwner(uint256 subId) external view returns (address) {
+        return pendingSubscriptionOwners[subId];
+    }
+
+    /// @notice Allows contract to receive native tokens when subscription is cancelled
+    receive() external payable {
+        // This function allows the contract to receive ETH when cancelSubscription
+        // sends the subscription balance to the adapter contract
+    }
+
+    /// @notice Internal function to transfer native tokens with proper validation
+    /// @param to Address to send tokens to
+    /// @param amount Amount of native tokens to transfer
+    function _transferNative(address to, uint256 amount) internal {
+        require(to != address(0), "Invalid transfer address");
+        require(amount > 0, "Amount must be greater than 0");
+
+        (bool success,) = to.call{value: amount}("");
+        require(success, "Transfer failed");
+    }
 }
diff --git a/src/signature-requests/SignatureSender.sol b/src/signature-requests/SignatureSender.sol
@@ -6,7 +6,7 @@ import {AccessControlEnumerableUpgradeable} from
 import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
 import {ContextUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol";
-import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol";
+import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
 import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 
 import {TypesLib} from "../libraries/TypesLib.sol";
@@ -30,7 +30,7 @@ contract SignatureSender is
     Initializable,
     UUPSUpgradeable,
     AccessControlEnumerableUpgradeable,
-    ReentrancyGuardUpgradeable
+    ReentrancyGuard
 {
     using BytesLib for bytes;
     using EnumerableSet for EnumerableSet.UintSet;
@@ -93,7 +93,6 @@ contract SignatureSender is
     function initialize(address owner, address _signatureSchemeAddressProvider) public initializer {
         __UUPSUpgradeable_init();
         __AccessControlEnumerable_init();
-        __ReentrancyGuard_init();
 
         require(_grantRole(ADMIN_ROLE, owner), "Grant role failed");
         require(_grantRole(DEFAULT_ADMIN_ROLE, owner), "Grant role reverts");
@@ -165,13 +164,14 @@ contract SignatureSender is
 
         require(sigScheme.verifySignature(request.messageHash, signature), "Signature verification failed");
 
+        // Apply CEI pattern: Update state before external interaction
+        requests[requestID].isFulfilled = true;
+        unfulfilledRequestIds.remove(requestID);
+
         (bool success,) = request.callback.call(
             abi.encodeWithSelector(ISignatureReceiver.receiveSignature.selector, requestID, signature)
         );
 
-        requests[requestID].isFulfilled = true;
-        unfulfilledRequestIds.remove(requestID);
-
         if (!success) {
             erroredRequestIds.add(requestID);
             emit SignatureCallbackFailed(requestID);
diff --git a/test/forge/ChainlinkVRFV2_5Integration_Subscription.t.sol b/test/forge/ChainlinkVRFV2_5Integration_Subscription.t.sol
@@ -132,4 +132,212 @@ contract ChainlinkVRFV2_5Integration_SubscriptionTest is Deployment {
 
         console.log("received randomness", consumer.getRandomWords(requestId)[0]);
     }
+
+    /// @notice Tests successful subscription ownership transfer flow
+    function test_subscriptionOwnershipTransfer_successful() public {
+        // Deploy wrapper adapter
+        ChainlinkVRFCoordinatorV2_5Adapter wrapper =
+            new ChainlinkVRFCoordinatorV2_5Adapter(admin, address(randomnessSender));
+
+        // Create subscription as alice
+        vm.prank(alice);
+        uint256 subId = wrapper.createSubscription();
+
+        // Verify initial state
+        assertTrue(wrapper.getPendingSubscriptionOwner(subId) == address(0), "No pending owner initially");
+
+        // Request ownership transfer to bob
+        vm.prank(alice);
+        vm.expectEmit(true, false, false, true);
+        emit ChainlinkVRFCoordinatorV2_5Adapter.SubscriptionOwnerTransferRequested(subId, alice, bob);
+        wrapper.requestSubscriptionOwnerTransfer(subId, bob);
+
+        // Verify pending state
+        assertTrue(wrapper.getPendingSubscriptionOwner(subId) == bob, "Bob should be pending owner");
+
+        // Accept ownership transfer as bob
+        vm.prank(bob);
+        vm.expectEmit(true, false, false, true);
+        emit ChainlinkVRFCoordinatorV2_5Adapter.SubscriptionOwnerTransferred(subId, alice, bob);
+        wrapper.acceptSubscriptionOwnerTransfer(subId);
+
+        // Verify ownership transfer completed
+        assertTrue(wrapper.getPendingSubscriptionOwner(subId) == address(0), "No pending owner after transfer");
+
+        // Verify bob can now manage the subscription
+        vm.prank(bob);
+        wrapper.addConsumer(subId, makeAddr("new-consumer"));
+
+        // Verify alice can no longer manage the subscription
+        vm.prank(alice);
+        vm.expectRevert("Caller is not subscription owner");
+        wrapper.addConsumer(subId, makeAddr("another-consumer"));
+    }
+
+    /// @notice Tests that non-owner cannot request ownership transfer
+    function test_subscriptionOwnershipTransfer_onlyOwnerCanRequest() public {
+        ChainlinkVRFCoordinatorV2_5Adapter wrapper =
+            new ChainlinkVRFCoordinatorV2_5Adapter(admin, address(randomnessSender));
+
+        vm.prank(alice);
+        uint256 subId = wrapper.createSubscription();
+
+        // Try to request transfer as non-owner (bob)
+        vm.prank(bob);
+        vm.expectRevert("Caller is not subscription owner");
+        wrapper.requestSubscriptionOwnerTransfer(subId, bob);
+    }
+
+    /// @notice Tests that only pending owner can accept transfer
+    function test_subscriptionOwnershipTransfer_onlyPendingOwnerCanAccept() public {
+        ChainlinkVRFCoordinatorV2_5Adapter wrapper =
+            new ChainlinkVRFCoordinatorV2_5Adapter(admin, address(randomnessSender));
+
+        vm.prank(alice);
+        uint256 subId = wrapper.createSubscription();
+
+        // Request transfer to bob
+        vm.prank(alice);
+        wrapper.requestSubscriptionOwnerTransfer(subId, bob);
+
+        // Try to accept as charlie (not the pending owner)
+        vm.prank(charlie);
+        vm.expectRevert("Caller is not the pending owner");
+        wrapper.acceptSubscriptionOwnerTransfer(subId);
+
+        // Try to accept as alice (current owner, not pending owner)
+        vm.prank(alice);
+        vm.expectRevert("Caller is not the pending owner");
+        wrapper.acceptSubscriptionOwnerTransfer(subId);
+    }
+
+    /// @notice Tests that accepting transfer without pending transfer fails
+    function test_subscriptionOwnershipTransfer_noPendingTransfer() public {
+        ChainlinkVRFCoordinatorV2_5Adapter wrapper =
+            new ChainlinkVRFCoordinatorV2_5Adapter(admin, address(randomnessSender));
+
+        vm.prank(alice);
+        uint256 subId = wrapper.createSubscription();
+
+        // Try to accept transfer without requesting it first
+        vm.prank(bob);
+        vm.expectRevert("No pending ownership transfer");
+        wrapper.acceptSubscriptionOwnerTransfer(subId);
+    }
+
+    /// @notice Tests that requesting transfer to zero address fails
+    function test_subscriptionOwnershipTransfer_zeroAddressRevert() public {
+        ChainlinkVRFCoordinatorV2_5Adapter wrapper =
+            new ChainlinkVRFCoordinatorV2_5Adapter(admin, address(randomnessSender));
+
+        vm.prank(alice);
+        uint256 subId = wrapper.createSubscription();
+
+        // Try to request transfer to zero address
+        vm.prank(alice);
+        vm.expectRevert("New owner cannot be zero address");
+        wrapper.requestSubscriptionOwnerTransfer(subId, address(0));
+    }
+
+    /// @notice Tests that requesting transfer to same owner fails
+    function test_subscriptionOwnershipTransfer_sameOwnerRevert() public {
+        ChainlinkVRFCoordinatorV2_5Adapter wrapper =
+            new ChainlinkVRFCoordinatorV2_5Adapter(admin, address(randomnessSender));
+
+        vm.prank(alice);
+        uint256 subId = wrapper.createSubscription();
+
+        // Try to request transfer to same owner
+        vm.prank(alice);
+        vm.expectRevert("New owner is the same as current owner");
+        wrapper.requestSubscriptionOwnerTransfer(subId, alice);
+    }
+
+    /// @notice Tests that pending transfer is cleared when subscription is cancelled
+    function test_subscriptionOwnershipTransfer_clearedOnCancellation() public {
+        ChainlinkVRFCoordinatorV2_5Adapter wrapper =
+            new ChainlinkVRFCoordinatorV2_5Adapter(admin, address(randomnessSender));
+
+        vm.prank(alice);
+        uint256 subId = wrapper.createSubscription();
+
+        // Fund subscription
+        vm.prank(alice);
+        wrapper.fundSubscriptionWithNative{value: 1 ether}(subId);
+
+        // Request ownership transfer
+        vm.prank(alice);
+        wrapper.requestSubscriptionOwnerTransfer(subId, bob);
+
+        // Verify pending transfer exists
+        assertTrue(wrapper.getPendingSubscriptionOwner(subId) == bob, "Bob should be pending owner");
+
+        // Cancel subscription
+        uint96 preBalance = uint96(alice.balance);
+        (, uint96 subBalance,,,) = wrapper.getSubscription(subId);
+        vm.prank(alice);
+        wrapper.cancelSubscription(subId, alice);
+
+        // Check that alice received the subscription balance
+        assertTrue(uint96(alice.balance) >= preBalance + subBalance, "Alice should receive the subscription balance");
+
+        // Verify pending transfer is cleared
+        assertTrue(wrapper.getPendingSubscriptionOwner(subId) == address(0), "Pending owner should be cleared");
+    }
+
+    /// @notice Tests that adapter remains owner in underlying SubscriptionAPI after transfer
+    function test_subscriptionOwnershipTransfer_adapterRemainsUnderlyingOwner() public {
+        ChainlinkVRFCoordinatorV2_5Adapter wrapper =
+            new ChainlinkVRFCoordinatorV2_5Adapter(admin, address(randomnessSender));
+
+        vm.prank(alice);
+        uint256 subId = wrapper.createSubscription();
+
+        // Transfer ownership to bob
+        vm.prank(alice);
+        wrapper.requestSubscriptionOwnerTransfer(subId, bob);
+
+        vm.prank(bob);
+        wrapper.acceptSubscriptionOwnerTransfer(subId);
+
+        // Verify that the wrapper is still the owner in the underlying SubscriptionAPI
+        (,, address underlyingOwner,) = randomnessSender.getSubscription(subId);
+        assertTrue(underlyingOwner == address(wrapper), "Wrapper should remain the owner in underlying SubscriptionAPI");
+
+        // Verify that bob can manage the subscription through the wrapper
+        vm.prank(bob);
+        wrapper.addConsumer(subId, makeAddr("new-consumer"));
+
+        // Verify management functions work (no revert)
+        vm.prank(bob);
+        wrapper.removeConsumer(subId, makeAddr("new-consumer"));
+    }
+
+    /// @notice Tests that multiple ownership transfers can be requested (overwriting pending)
+    function test_subscriptionOwnershipTransfer_overwritePending() public {
+        ChainlinkVRFCoordinatorV2_5Adapter wrapper =
+            new ChainlinkVRFCoordinatorV2_5Adapter(admin, address(randomnessSender));
+
+        vm.prank(alice);
+        uint256 subId = wrapper.createSubscription();
+
+        // Request transfer to bob
+        vm.prank(alice);
+        wrapper.requestSubscriptionOwnerTransfer(subId, bob);
+        assertTrue(wrapper.getPendingSubscriptionOwner(subId) == bob, "Bob should be pending owner");
+
+        // Request transfer to charlie (should overwrite)
+        vm.prank(alice);
+        wrapper.requestSubscriptionOwnerTransfer(subId, charlie);
+        assertTrue(wrapper.getPendingSubscriptionOwner(subId) == charlie, "Charlie should be pending owner");
+
+        // Bob can no longer accept (since pending was overwritten)
+        vm.prank(bob);
+        vm.expectRevert("Caller is not the pending owner");
+        wrapper.acceptSubscriptionOwnerTransfer(subId);
+
+        // Charlie can accept
+        vm.prank(charlie);
+        wrapper.acceptSubscriptionOwnerTransfer(subId);
+    }
 }
diff --git a/test/forge/base/Base.t.sol b/test/forge/base/Base.t.sol
