From 3544c771408f136f3a55f5093fa9fd960ac26dfb Mon Sep 17 00:00:00 2001 From: LeoSlrRf Date: Mon, 24 Aug 2026 18:10:02 +0200 Subject: [PATCH] fix(recurring): grant and revoke relayer as separate calls --- .../contracts/ERC20RecurringPaymentProxy.sol | 35 ++++-- .../ERC20RecurringPaymentProxy.test.ts | 108 ++++++++++-------- 2 files changed, 86 insertions(+), 57 deletions(-) diff --git a/packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol b/packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol index 6be6e597b..6430008eb 100644 --- a/packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol +++ b/packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol @@ -6,7 +6,6 @@ import '@openzeppelin/contracts/security/Pausable.sol'; import '@openzeppelin/contracts/security/ReentrancyGuard.sol'; import '@openzeppelin/contracts/utils/cryptography/EIP712.sol'; import '@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol'; -import '@openzeppelin/contracts/access/Ownable.sol'; import './interfaces/ERC20FeeProxy.sol'; import './lib/SafeERC20.sol'; @@ -14,7 +13,7 @@ import './lib/SafeERC20.sol'; * @title ERC20RecurringPaymentProxy * @notice Triggers recurring ERC20 payments based on predefined schedules. */ -contract ERC20RecurringPaymentProxy is EIP712, AccessControl, Pausable, ReentrancyGuard, Ownable { +contract ERC20RecurringPaymentProxy is EIP712, AccessControl, Pausable, ReentrancyGuard { using SafeERC20 for IERC20; error ERC20RecurringPaymentProxy__BadSignature(); @@ -35,9 +34,12 @@ contract ERC20RecurringPaymentProxy is EIP712, AccessControl, Pausable, Reentran error ERC20RecurringPaymentProxy__NotSubscriber(); error ERC20RecurringPaymentProxy__Cancelled(); error ERC20RecurringPaymentProxy__NotAdmitted(); + error ERC20RecurringPaymentProxy__NotRelayer(); uint8 public constant MAX_LEGS = 8; + /// @notice Relayers may trigger any due cycle. Extra holders of this role compete + /// for `relayerFee` because `_payRelayer` pays `msg.sender`. bytes32 public constant RELAYER_ROLE = keccak256('RELAYER_ROLE'); bytes32 private constant _LEG_TYPEHASH = @@ -102,7 +104,6 @@ contract ERC20RecurringPaymentProxy is EIP712, AccessControl, Pausable, Reentran } _grantRole(DEFAULT_ADMIN_ROLE, adminSafe); _grantRole(RELAYER_ROLE, relayerEOA); - transferOwnership(adminSafe); erc20FeeProxy = IERC20FeeProxy(erc20FeeProxyAddress); } @@ -404,24 +405,36 @@ contract ERC20RecurringPaymentProxy is EIP712, AccessControl, Pausable, Reentran emit ScheduleCancelled(scheduleKey, p.subscriber); } - function setRelayer(address oldRelayer, address newRelayer) external onlyOwner { - if (newRelayer == address(0)) revert ERC20RecurringPaymentProxy__ZeroAddress(); - _revokeRole(RELAYER_ROLE, oldRelayer); - _grantRole(RELAYER_ROLE, newRelayer); + /** + * @notice Grants `RELAYER_ROLE`. Every holder can collect `relayerFee` on trigger. + */ + function grantRelayer(address relayer) external onlyRole(DEFAULT_ADMIN_ROLE) { + if (relayer == address(0)) revert ERC20RecurringPaymentProxy__ZeroAddress(); + _grantRole(RELAYER_ROLE, relayer); + } + + /** + * @notice Revokes `RELAYER_ROLE`. Reverts if `relayer` does not hold the role. + */ + function revokeRelayer(address relayer) external onlyRole(DEFAULT_ADMIN_ROLE) { + if (!hasRole(RELAYER_ROLE, relayer)) { + revert ERC20RecurringPaymentProxy__NotRelayer(); + } + _revokeRole(RELAYER_ROLE, relayer); } - function setFeeProxy(address newProxy) external onlyOwner { + function setFeeProxy(address newProxy) external onlyRole(DEFAULT_ADMIN_ROLE) { if (newProxy == address(0)) revert ERC20RecurringPaymentProxy__ZeroAddress(); address oldProxy = address(erc20FeeProxy); erc20FeeProxy = IERC20FeeProxy(newProxy); emit FeeProxyUpdated(oldProxy, newProxy); } - function pause() external onlyOwner { + function pause() external onlyRole(DEFAULT_ADMIN_ROLE) { _pause(); } - function unpause() external onlyOwner { + function unpause() external onlyRole(DEFAULT_ADMIN_ROLE) { _unpause(); } @@ -429,7 +442,7 @@ contract ERC20RecurringPaymentProxy is EIP712, AccessControl, Pausable, Reentran address token, address to, uint256 amount - ) external onlyOwner nonReentrant { + ) external onlyRole(DEFAULT_ADMIN_ROLE) nonReentrant { if (token == address(0) || to == address(0)) { revert ERC20RecurringPaymentProxy__ZeroAddress(); } diff --git a/packages/smart-contracts/test/contracts/ERC20RecurringPaymentProxy.test.ts b/packages/smart-contracts/test/contracts/ERC20RecurringPaymentProxy.test.ts index d8a984d9e..5916e83ed 100644 --- a/packages/smart-contracts/test/contracts/ERC20RecurringPaymentProxy.test.ts +++ b/packages/smart-contracts/test/contracts/ERC20RecurringPaymentProxy.test.ts @@ -119,7 +119,6 @@ describe('ERC20RecurringPaymentProxy', () => { it('should be deployed with correct initial values', async () => { expect(erc20RecurringPaymentProxy.address).to.not.equal(ethers.constants.AddressZero); expect(await erc20RecurringPaymentProxy.erc20FeeProxy()).to.equal(erc20FeeProxy.address); - expect(await erc20RecurringPaymentProxy.owner()).to.equal(ownerAddress); expect( await erc20RecurringPaymentProxy.hasRole( await erc20RecurringPaymentProxy.RELAYER_ROLE(), @@ -169,16 +168,16 @@ describe('ERC20RecurringPaymentProxy', () => { }); }); - describe('setRelayer', () => { - it('should allow owner to set new relayer', async () => { - await erc20RecurringPaymentProxy.setRelayer(relayerAddress, newRelayerAddress); + describe('grantRelayer and revokeRelayer', () => { + it('grants RELAYER_ROLE to a new address', async () => { + await erc20RecurringPaymentProxy.grantRelayer(newRelayerAddress); expect( await erc20RecurringPaymentProxy.hasRole( await erc20RecurringPaymentProxy.RELAYER_ROLE(), relayerAddress, ), - ).to.be.false; + ).to.be.true; expect( await erc20RecurringPaymentProxy.hasRole( await erc20RecurringPaymentProxy.RELAYER_ROLE(), @@ -187,18 +186,51 @@ describe('ERC20RecurringPaymentProxy', () => { ).to.be.true; }); - it('should revert when non-owner tries to set relayer', async () => { + it('revokes RELAYER_ROLE from a current relayer', async () => { + await erc20RecurringPaymentProxy.revokeRelayer(relayerAddress); + + expect( + await erc20RecurringPaymentProxy.hasRole( + await erc20RecurringPaymentProxy.RELAYER_ROLE(), + relayerAddress, + ), + ).to.be.false; + }); + + it('reverts when a non-admin tries to grant or revoke', async () => { + await expect( + erc20RecurringPaymentProxy.connect(user).grantRelayer(newRelayerAddress), + ).to.be.revertedWith('AccessControl: account'); await expect( - erc20RecurringPaymentProxy.connect(user).setRelayer(relayerAddress, newRelayerAddress), - ).to.be.revertedWith('Ownable: caller is not the owner'); + erc20RecurringPaymentProxy.connect(user).revokeRelayer(relayerAddress), + ).to.be.revertedWith('AccessControl: account'); }); - it('should emit RoleRevoked and RoleGranted events', async () => { - await expect(erc20RecurringPaymentProxy.setRelayer(relayerAddress, newRelayerAddress)) - .to.emit(erc20RecurringPaymentProxy, 'RoleRevoked') - .withArgs(await erc20RecurringPaymentProxy.RELAYER_ROLE(), relayerAddress, ownerAddress) - .and.to.emit(erc20RecurringPaymentProxy, 'RoleGranted') + it('emits RoleGranted and RoleRevoked', async () => { + await expect(erc20RecurringPaymentProxy.grantRelayer(newRelayerAddress)) + .to.emit(erc20RecurringPaymentProxy, 'RoleGranted') .withArgs(await erc20RecurringPaymentProxy.RELAYER_ROLE(), newRelayerAddress, ownerAddress); + + await expect(erc20RecurringPaymentProxy.revokeRelayer(relayerAddress)) + .to.emit(erc20RecurringPaymentProxy, 'RoleRevoked') + .withArgs(await erc20RecurringPaymentProxy.RELAYER_ROLE(), relayerAddress, ownerAddress); + }); + + it('reverts grant of the zero address', async () => { + await expect( + erc20RecurringPaymentProxy.grantRelayer(ethers.constants.AddressZero), + ).to.be.revertedWith('ERC20RecurringPaymentProxy__ZeroAddress'); + }); + + it('reverts revoke when the address does not hold RELAYER_ROLE and leaves holders unchanged', async () => { + const relayerRole = await erc20RecurringPaymentProxy.RELAYER_ROLE(); + + await expect(erc20RecurringPaymentProxy.revokeRelayer(userAddress)).to.be.revertedWith( + 'ERC20RecurringPaymentProxy__NotRelayer', + ); + + expect(await erc20RecurringPaymentProxy.hasRole(relayerRole, relayerAddress)).to.be.true; + expect(await erc20RecurringPaymentProxy.hasRole(relayerRole, userAddress)).to.be.false; }); }); @@ -219,7 +251,7 @@ describe('ERC20RecurringPaymentProxy', () => { await expect( erc20RecurringPaymentProxy.connect(user).setFeeProxy(newERC20FeeProxy.address), - ).to.be.revertedWith('Ownable: caller is not the owner'); + ).to.be.revertedWith('AccessControl: account'); }); it('should revert when trying to set zero address as fee proxy', async () => { @@ -244,7 +276,7 @@ describe('ERC20RecurringPaymentProxy', () => { it('should revert when non-owner tries to pause', async () => { await expect(erc20RecurringPaymentProxy.connect(user).pause()).to.be.revertedWith( - 'Ownable: caller is not the owner', + 'AccessControl: account', ); }); @@ -252,7 +284,7 @@ describe('ERC20RecurringPaymentProxy', () => { await erc20RecurringPaymentProxy.pause(); await expect(erc20RecurringPaymentProxy.connect(user).unpause()).to.be.revertedWith( - 'Ownable: caller is not the owner', + 'AccessControl: account', ); }); @@ -271,38 +303,22 @@ describe('ERC20RecurringPaymentProxy', () => { }); }); - describe('Ownership', () => { - it('should allow owner to transfer ownership', async () => { - await erc20RecurringPaymentProxy.transferOwnership(newOwnerAddress); - expect(await erc20RecurringPaymentProxy.owner()).to.equal(newOwnerAddress); - }); - - it('should revert when non-owner tries to transfer ownership', async () => { - await expect( - erc20RecurringPaymentProxy.connect(user).transferOwnership(newOwnerAddress), - ).to.be.revertedWith('Ownable: caller is not the owner'); - }); - - it('should emit OwnershipTransferred event', async () => { - await expect(erc20RecurringPaymentProxy.transferOwnership(newOwnerAddress)) - .to.emit(erc20RecurringPaymentProxy, 'OwnershipTransferred') - .withArgs(ownerAddress, newOwnerAddress); - }); - - it('should allow new owner to renounce ownership', async () => { - await erc20RecurringPaymentProxy.transferOwnership(newOwnerAddress); - - await expect(erc20RecurringPaymentProxy.connect(newOwner).renounceOwnership()) - .to.emit(erc20RecurringPaymentProxy, 'OwnershipTransferred') - .withArgs(newOwnerAddress, ethers.constants.AddressZero); + describe('Admin role', () => { + it('lets the admin grant and revoke DEFAULT_ADMIN_ROLE', async () => { + const adminRole = await erc20RecurringPaymentProxy.DEFAULT_ADMIN_ROLE(); + await erc20RecurringPaymentProxy.grantRole(adminRole, newOwnerAddress); + expect(await erc20RecurringPaymentProxy.hasRole(adminRole, newOwnerAddress)).to.be.true; - expect(await erc20RecurringPaymentProxy.owner()).to.equal(ethers.constants.AddressZero); + await erc20RecurringPaymentProxy.connect(newOwner).revokeRole(adminRole, ownerAddress); + expect(await erc20RecurringPaymentProxy.hasRole(adminRole, ownerAddress)).to.be.false; }); - it('should revert when non-owner tries to renounce ownership', async () => { - await expect(erc20RecurringPaymentProxy.connect(user).renounceOwnership()).to.be.revertedWith( - 'Ownable: caller is not the owner', - ); + it('reverts when a non-admin tries to grant admin', async () => { + await expect( + erc20RecurringPaymentProxy + .connect(user) + .grantRole(await erc20RecurringPaymentProxy.DEFAULT_ADMIN_ROLE(), userAddress), + ).to.be.revertedWith('AccessControl: account'); }); }); @@ -322,7 +338,7 @@ describe('ERC20RecurringPaymentProxy', () => { await expect( erc20RecurringPaymentProxy.connect(user).rescueTokens(testERC20.address, userAddress, 10), - ).to.be.revertedWith('Ownable: caller is not the owner'); + ).to.be.revertedWith('AccessControl: account'); }); it('reverts rescue to the zero address', async () => {