From 7d209d8f3d5a3b49451907fb7e06dba4cb011533 Mon Sep 17 00:00:00 2001 From: Theo Butler Date: Tue, 14 Feb 2023 10:54:40 -0500 Subject: [PATCH] contract: Fix any user modifying authorizedSigners for another user --- contract/build/Subscriptions.abi | 10 ------- contract/contracts/Subscriptions.sol | 24 ++++++++-------- contract/test/contract.test.ts | 42 ++++++++++++++++++---------- 3 files changed, 40 insertions(+), 36 deletions(-) diff --git a/contract/build/Subscriptions.abi b/contract/build/Subscriptions.abi index df47a37..9dc1e6c 100644 --- a/contract/build/Subscriptions.abi +++ b/contract/build/Subscriptions.abi @@ -230,11 +230,6 @@ }, { "inputs": [ - { - "internalType": "address", - "name": "_user", - "type": "address" - }, { "internalType": "address", "name": "_signer", @@ -505,11 +500,6 @@ }, { "inputs": [ - { - "internalType": "address", - "name": "_user", - "type": "address" - }, { "internalType": "address", "name": "_signer", diff --git a/contract/contracts/Subscriptions.sol b/contract/contracts/Subscriptions.sol index 67484b0..dcd0913 100644 --- a/contract/contracts/Subscriptions.sol +++ b/contract/contracts/Subscriptions.sol @@ -270,22 +270,22 @@ contract Subscriptions is Ownable { } } - /// @param _user Subscription owner. - /// @param _signer Address to be authorized to sign messages on the owners behalf. - function addAuthorizedSigner(address _user, address _signer) public { - require(_user != _signer, 'user is always an authorized signer'); - authorizedSigners[_user][_signer] = true; + /// @param _signer Address to be authorized to sign messages on the sender's behalf. + function addAuthorizedSigner(address _signer) public { + address user = msg.sender; + require(user != _signer, 'user is always an authorized signer'); + authorizedSigners[user][_signer] = true; - emit AuthorizedSignerAdded(_user, _signer); + emit AuthorizedSignerAdded(user, _signer); } - /// @param _user Subscription owner. - /// @param _signer Address to become unauthorized to sign messages on the owners behalf. - function removeAuthorizedSigner(address _user, address _signer) public { - require(_user != _signer, 'user is always an authorized signer'); - delete authorizedSigners[_user][_signer]; + /// @param _signer Address to become unauthorized to sign messages on the sender's behalf. + function removeAuthorizedSigner(address _signer) public { + address user = msg.sender; + require(user != _signer, 'user is always an authorized signer'); + delete authorizedSigners[user][_signer]; - emit AuthorizedSignerRemoved(_user, _signer); + emit AuthorizedSignerRemoved(user, _signer); } /// @param _user Subscription owner. diff --git a/contract/test/contract.test.ts b/contract/test/contract.test.ts index e204601..a69f3ae 100644 --- a/contract/test/contract.test.ts +++ b/contract/test/contract.test.ts @@ -98,12 +98,12 @@ describe('Subscriptions contract', () => { it('user is always authorized', async function () { const user = subscriber1.address; expect(await subscriptions.checkAuthorizedSigner(user, user)).to.eq(true); - await expect(subscriptions.addAuthorizedSigner(user, user)).revertedWith( - 'user is always an authorized signer' - ); - await expect(subscriptions.removeAuthorizedSigner(user, user)).revertedWith( - 'user is always an authorized signer' - ); + await expect( + subscriptions.connect(subscriber1.signer).addAuthorizedSigner(user) + ).revertedWith('user is always an authorized signer'); + await expect( + subscriptions.connect(subscriber1.signer).removeAuthorizedSigner(user) + ).revertedWith('user is always an authorized signer'); }); it('other addresses are unauthorized by default', async function () { @@ -117,7 +117,9 @@ describe('Subscriptions contract', () => { it('authorizedSigners can be added', async function () { const user = subscriber1.address; const other = subscriber2.address; - const tx = await subscriptions.addAuthorizedSigner(user, other); + const tx = await subscriptions + .connect(subscriber1.signer) + .addAuthorizedSigner(other); expect(await subscriptions.checkAuthorizedSigner(user, other)).to.eq( true ); @@ -129,8 +131,12 @@ describe('Subscriptions contract', () => { it('authorizedSigners can be removed', async function () { const user = subscriber1.address; const other = subscriber2.address; - await subscriptions.addAuthorizedSigner(user, other); - const tx = await subscriptions.removeAuthorizedSigner(user, other); + await subscriptions + .connect(subscriber1.signer) + .addAuthorizedSigner(other); + const tx = await subscriptions + .connect(subscriber1.signer) + .removeAuthorizedSigner(other); expect(await subscriptions.checkAuthorizedSigner(user, other)).to.eq( false ); @@ -718,7 +724,9 @@ describe('Subscriptions contract', () => { .connect(subscriber2.signer) .setPendingSubscription(subscriber1.address, start, end, rate); - await expect(tx).revertedWith('Can only set pending subscriptions for self'); + await expect(tx).revertedWith( + 'Can only set pending subscriptions for self' + ); }); }); @@ -776,7 +784,9 @@ describe('Subscriptions contract', () => { }); it('should revert if there is no pending subscription', async function () { - const tx = subscriptions.connect(subscriber1.signer).fulfil(subscriber2.address, oneMillion); + const tx = subscriptions + .connect(subscriber1.signer) + .fulfil(subscriber2.address, oneMillion); await expect(tx).revertedWith('No pending subscription'); }); @@ -795,8 +805,12 @@ describe('Subscriptions contract', () => { .connect(subscriber2.signer) .approve(subscriptions.address, value); - const tx = subscriptions.connect(subscriber2.signer).fulfil(subscriber1.address, zero); - await expect(tx).revertedWith('Insufficient funds to create subscription'); + const tx = subscriptions + .connect(subscriber2.signer) + .fulfil(subscriber1.address, zero); + await expect(tx).revertedWith( + 'Insufficient funds to create subscription' + ); }); }); }); @@ -1175,7 +1189,7 @@ async function fulfil( // Actual amount deposited might be less than intended if subStart < block.number const amountDeposited = rate.mul(end.sub(start)); const amountLeftover = amount.sub(amountDeposited); - + expect(afterContractBalance).to.eq( beforeContractBalance.add(amountDeposited) );