Skip to content

fix(recurring): grant and revoke relayer as separate calls #1761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
LeoSlrRf wants to merge 1 commit into from
+86 −57
Closed

fix(recurring): grant and revoke relayer as separate calls #1761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 24 additions & 11 deletions packages/smart-contracts/src/contracts/ERC20RecurringPaymentProxy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,14 @@ 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';

/**
* @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();
Expand All @@ -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 =
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -404,32 +405,44 @@ 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);
}
Comment on lines +411 to 424

@greptile-apps greptile-apps Bot Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Published ABI misses relayer methods

When clients connect through the published erc20RecurringPaymentProxyArtifact, its ABI lacks grantRelayer and revokeRelayer while retaining the removed setRelayer and Ownable methods, causing new operations to be unavailable and obsolete calls to revert against bytecode built from this source. Update the committed versioned artifact alongside this public API change.

Knowledge Base Used: Smart contracts and deployments

@LeoSlrRf LeoSlrRf Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New ABI is published in a PR up the graphite stack


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();
}

function rescueTokens(
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();
}
Expand Down
108 changes: 62 additions & 46 deletions packages/smart-contracts/test/contracts/ERC20RecurringPaymentProxy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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(),
Expand All @@ -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;
});
});

Expand All @@ -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 () => {
Expand All @@ -244,15 +276,15 @@ 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',
);
});

it('should revert when non-owner tries to unpause', async () => {
await erc20RecurringPaymentProxy.pause();

await expect(erc20RecurringPaymentProxy.connect(user).unpause()).to.be.revertedWith(
'Ownable: caller is not the owner',
'AccessControl: account',
);
});

Expand All @@ -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');
});
});

Expand All @@ -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 () => {
Expand Down