From 7c6ec59245c6de72f6f813fc152869540c585f59 Mon Sep 17 00:00:00 2001 From: Miguel de Elias Date: Wed, 6 Sep 2023 00:02:09 +1000 Subject: [PATCH 1/6] fix: denial of service attack on addToL2WithPermit function --- contracts/BillingConnector.sol | 35 ++++++++++++- test/billingConnector.test.ts | 93 ++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 1 deletion(-) diff --git a/contracts/BillingConnector.sol b/contracts/BillingConnector.sol index 25c0379..7301eb4 100644 --- a/contracts/BillingConnector.sol +++ b/contracts/BillingConnector.sol @@ -178,7 +178,7 @@ contract BillingConnector is IBillingConnector, Governed, Rescuable, L1ArbitrumM ) external payable override { require(_amount != 0, "Must add more than 0"); require(_user != address(0), "destination != 0"); - IERC20WithPermit(address(graphToken)).permit(_user, address(this), _amount, _deadline, _v, _r, _s); + _permit(_user, address(this), _amount, _deadline, _v, _r, _s); _addToL2(_user, _user, _amount, _maxGas, _gasPriceBid, _maxSubmissionCost); } @@ -261,4 +261,37 @@ contract BillingConnector is IBillingConnector, Governed, Rescuable, L1ArbitrumM inbox = _inbox; emit ArbitrumInboxUpdated(_inbox); } + + /** + * @dev Approve token allowance by validating a message signed by the holder, if permit + * fails check for existing allowance before reverting transaction. + * @param _owner Address of the token holder + * @param _spender Address of the approved spender + * @param _value Amount of tokens to approve the spender + * @param _deadline Expiration time of the signed permit (if zero, the permit will never expire, so use with caution) + * @param _v Signature recovery id + * @param _r Signature r value + * @param _s Signature s value + */ + function _permit( + address _owner, + address _spender, + uint256 _value, + uint256 _deadline, + uint8 _v, + bytes32 _r, + bytes32 _s + ) internal { + // Try permit() before allowance check to advance nonce if possible + try IERC20WithPermit(address(graphToken)).permit(_owner, _spender, _value, _deadline, _v, _r, _s) { + return; + } catch Error(string memory reason) { + // Check for existing allowance before reverting + if (IERC20(address(graphToken)).allowance(_owner, _spender) >= _value) { + return; + } + + revert(reason); + } + } } diff --git a/test/billingConnector.test.ts b/test/billingConnector.test.ts index e89ab5f..a902736 100644 --- a/test/billingConnector.test.ts +++ b/test/billingConnector.test.ts @@ -341,6 +341,99 @@ describe('BillingConnector', () => { expect(connectorBalanceAfter).eq(connectorBalanceBefore) expect(bridgeBalanceAfter).eq(bridgeBalanceBefore.add(oneHundred)) }) + it("doesn't revert after a frontrunning permit transaction", async function () { + const permit = await permitOK(oneHundred, me.address, billingConnector.address) + const signedPermit = createSignedPermit(permit, myPrivateKey, SALT) + + const myBalanceBefore = await token.balanceOf(me.address) + const userBalanceBefore = await token.balanceOf(user1.address) + const connectorBalanceBefore = await token.balanceOf(billingConnector.address) + const bridgeBalanceBefore = await token.balanceOf(l1TokenGatewayMock.address) + + // Disable automining to execute two transactions in the same block + await hre.network.provider.send('evm_setAutomine', [false]) + + const tx = await billingConnector + .connect(user1.signer) + .addToL2WithPermit( + me.address, + permit.value, + defaultMaxGas, + defaultGasPriceBid, + defaultMaxSubmissionPrice, + permit.deadline, + signedPermit.v, + signedPermit.r, + signedPermit.s, + { + value: defaultMsgValue, + }, + ) + + const expectedCallhookData = defaultAbiCoder.encode(['address'], [me.address]) + const expectedOutboundCalldata = l1TokenGatewayMock.interface.encodeFunctionData('finalizeInboundTransfer', [ + token.address, + billingConnector.address, + l2BillingMock.address, + oneHundred, + defaultAbiCoder.encode(['bytes', 'bytes'], ['0x', expectedCallhookData]), + ]) + + // user2 frontruns transaction with their own permit transaction by increasing gas price + const frontrunnerTx = await token + .connect(user2.signer) + .permit( + me.address, + billingConnector.address, + permit.value, + permit.deadline, + signedPermit.v, + signedPermit.r, + signedPermit.s, + { + gasPrice: tx.gasPrice.add(100), + gasLimit: tx.gasLimit.add(1000), + }, + ) + + // manually mine all transactions + await hre.network.provider.send('evm_mine', []) + + // enable automining cleanup + await hre.network.provider.send('evm_setAutomine', [true]) + + // frontrunnerTx approves funds + await expect(frontrunnerTx) + .to.emit(token, 'Approval') + .withArgs(me.address, billingConnector.address, permit.value) + + // Transaction doesn't revert, instead it uses the allowance created by frontrunnerTx + await expect(tx).not.to.be.reverted + + // Real event emitted by BillingConnector + await expect(tx).emit(billingConnector, 'TokensSentToL2').withArgs(me.address, me.address, oneHundred) + // Mock event from the gateway mock to validate what would be sent to L2 + await expect(tx) + .emit(l1TokenGatewayMock, 'FakeTxToL2') + .withArgs( + billingConnector.address, + defaultMsgValue, + defaultMaxGas, + defaultGasPriceBid, + defaultMaxSubmissionPrice, + expectedOutboundCalldata, + ) + + // Balances are fine since transaction was executed fine + const myBalanceAfter = await token.balanceOf(me.address) + const userBalanceAfter = await token.balanceOf(user1.address) + const connectorBalanceAfter = await token.balanceOf(billingConnector.address) + const bridgeBalanceAfter = await token.balanceOf(l1TokenGatewayMock.address) + expect(myBalanceAfter).eq(myBalanceBefore.sub(oneHundred)) + expect(userBalanceAfter).eq(userBalanceBefore) // unchanged + expect(connectorBalanceAfter).eq(connectorBalanceBefore) + expect(bridgeBalanceAfter).eq(bridgeBalanceBefore.add(oneHundred)) + }) it("relies on the token's validation when the sender has provided an expired permit", async function () { // Note the permit is from me to billingConnector, but the addToL2WithPermit tx is signed by user1 const permit = await permitExpired(me.address, billingConnector.address) From f0f2d5529d5b1f0bebff4a5139c11cec8b4b9887 Mon Sep 17 00:00:00 2001 From: Miguel de Elias Date: Wed, 6 Sep 2023 10:43:39 +1000 Subject: [PATCH 2/6] fix: pr comments cleanup --- contracts/BillingConnector.sol | 5 +++-- test/billingConnector.test.ts | 38 ++++++++++++---------------------- 2 files changed, 16 insertions(+), 27 deletions(-) diff --git a/contracts/BillingConnector.sol b/contracts/BillingConnector.sol index 7301eb4..e68c92b 100644 --- a/contracts/BillingConnector.sol +++ b/contracts/BillingConnector.sol @@ -282,12 +282,13 @@ contract BillingConnector is IBillingConnector, Governed, Rescuable, L1ArbitrumM bytes32 _r, bytes32 _s ) internal { + IERC20WithPermit token = IERC20WithPermit(address(graphToken)); // Try permit() before allowance check to advance nonce if possible - try IERC20WithPermit(address(graphToken)).permit(_owner, _spender, _value, _deadline, _v, _r, _s) { + try token.permit(_owner, _spender, _value, _deadline, _v, _r, _s) { return; } catch Error(string memory reason) { // Check for existing allowance before reverting - if (IERC20(address(graphToken)).allowance(_owner, _spender) >= _value) { + if (token.allowance(_owner, _spender) >= _value) { return; } diff --git a/test/billingConnector.test.ts b/test/billingConnector.test.ts index a902736..a9d6af2 100644 --- a/test/billingConnector.test.ts +++ b/test/billingConnector.test.ts @@ -350,9 +350,20 @@ describe('BillingConnector', () => { const connectorBalanceBefore = await token.balanceOf(billingConnector.address) const bridgeBalanceBefore = await token.balanceOf(l1TokenGatewayMock.address) - // Disable automining to execute two transactions in the same block - await hre.network.provider.send('evm_setAutomine', [false]) + // user2 frontruns transaction + const frontrunnerTx = await token + .connect(user2.signer) + .permit( + me.address, + billingConnector.address, + permit.value, + permit.deadline, + signedPermit.v, + signedPermit.r, + signedPermit.s, + ) + // my original transaction run by user1 with my permit const tx = await billingConnector .connect(user1.signer) .addToL2WithPermit( @@ -379,29 +390,6 @@ describe('BillingConnector', () => { defaultAbiCoder.encode(['bytes', 'bytes'], ['0x', expectedCallhookData]), ]) - // user2 frontruns transaction with their own permit transaction by increasing gas price - const frontrunnerTx = await token - .connect(user2.signer) - .permit( - me.address, - billingConnector.address, - permit.value, - permit.deadline, - signedPermit.v, - signedPermit.r, - signedPermit.s, - { - gasPrice: tx.gasPrice.add(100), - gasLimit: tx.gasLimit.add(1000), - }, - ) - - // manually mine all transactions - await hre.network.provider.send('evm_mine', []) - - // enable automining cleanup - await hre.network.provider.send('evm_setAutomine', [true]) - // frontrunnerTx approves funds await expect(frontrunnerTx) .to.emit(token, 'Approval') From 60c71fa3de590918f1241137e9c8b242128082d7 Mon Sep 17 00:00:00 2001 From: Miguel de Elias Date: Fri, 8 Sep 2023 12:04:13 +1000 Subject: [PATCH 3/6] fix: only tokens owner can call addToL2WithPermit --- contracts/BillingConnector.sol | 4 +++- test/billingConnector.test.ts | 41 +++++++++++++++++++++++++--------- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/contracts/BillingConnector.sol b/contracts/BillingConnector.sol index e68c92b..ac3296d 100644 --- a/contracts/BillingConnector.sol +++ b/contracts/BillingConnector.sol @@ -154,7 +154,8 @@ contract BillingConnector is IBillingConnector, Governed, Rescuable, L1ArbitrumM } /** - * @notice Add tokens into the billing contract on L2, for any user, using a signed permit + * @notice Add tokens into the billing contract on L2 using a signed permit + * @dev _user must be the msg.sender * @param _user Address of the current owner of the tokens, that will also be the destination in L2 * @param _amount Amount of tokens to add * @param _maxGas Max gas for the L2 retryable ticket execution @@ -178,6 +179,7 @@ contract BillingConnector is IBillingConnector, Governed, Rescuable, L1ArbitrumM ) external payable override { require(_amount != 0, "Must add more than 0"); require(_user != address(0), "destination != 0"); + require(_user == msg.sender, "Only tokens owner can call"); _permit(_user, address(this), _amount, _deadline, _v, _r, _s); _addToL2(_user, _user, _amount, _maxGas, _gasPriceBid, _maxSubmissionCost); } diff --git a/test/billingConnector.test.ts b/test/billingConnector.test.ts index a9d6af2..2be1885 100644 --- a/test/billingConnector.test.ts +++ b/test/billingConnector.test.ts @@ -296,7 +296,7 @@ describe('BillingConnector', () => { const bridgeBalanceBefore = await token.balanceOf(l1TokenGatewayMock.address) const tx = billingConnector - .connect(user1.signer) + .connect(me.signer) .addToL2WithPermit( me.address, oneHundred, @@ -363,9 +363,9 @@ describe('BillingConnector', () => { signedPermit.s, ) - // my original transaction run by user1 with my permit + // my original transaction with permit const tx = await billingConnector - .connect(user1.signer) + .connect(me.signer) .addToL2WithPermit( me.address, permit.value, @@ -423,12 +423,11 @@ describe('BillingConnector', () => { expect(bridgeBalanceAfter).eq(bridgeBalanceBefore.add(oneHundred)) }) it("relies on the token's validation when the sender has provided an expired permit", async function () { - // Note the permit is from me to billingConnector, but the addToL2WithPermit tx is signed by user1 const permit = await permitExpired(me.address, billingConnector.address) const signedPermit = createSignedPermit(permit, myPrivateKey, SALT) const tx = billingConnector - .connect(user1.signer) + .connect(me.signer) .addToL2WithPermit( me.address, permit.value, @@ -450,7 +449,7 @@ describe('BillingConnector', () => { const signedPermit = createSignedPermit(permit, myPrivateKey, SALT) const tx = billingConnector - .connect(user1.signer) + .connect(me.signer) .addToL2WithPermit( me.address, permit.value, @@ -472,7 +471,7 @@ describe('BillingConnector', () => { const signedPermit = createSignedPermit(permit, myPrivateKey, SALT) const tx = billingConnector - .connect(user1.signer) + .connect(me.signer) .addToL2WithPermit( me.address, permit.value, @@ -494,7 +493,7 @@ describe('BillingConnector', () => { const signedPermit = createSignedPermit(permit, myPrivateKey, SALT) const tx = billingConnector - .connect(user1.signer) + .connect(me.signer) .addToL2WithPermit( AddressZero, permit.value, @@ -516,7 +515,7 @@ describe('BillingConnector', () => { const signedPermit = createSignedPermit(permit, myPrivateKey, SALT) const tx = billingConnector - .connect(user1.signer) + .connect(me.signer) .addToL2WithPermit( me.address, permit.value, @@ -538,7 +537,7 @@ describe('BillingConnector', () => { const signedPermit = createSignedPermit(permit, myPrivateKey, SALT) const tx = billingConnector - .connect(user1.signer) + .connect(me.signer) .addToL2WithPermit( me.address, permit.value, @@ -555,6 +554,28 @@ describe('BillingConnector', () => { ) await expect(tx).revertedWith('WRONG_ETH_VALUE') }) + it('rejects calls if not tokens owner', async function () { + const permit = await permitOK(oneHundred, me.address, billingConnector.address) + const signedPermit = createSignedPermit(permit, myPrivateKey, SALT) + + const tx = billingConnector + .connect(user1.signer) + .addToL2WithPermit( + me.address, + permit.value, + defaultMaxGas, + defaultGasPriceBid, + defaultMaxSubmissionPrice, + permit.deadline, + signedPermit.v, + signedPermit.r, + signedPermit.s, + { + value: defaultMsgValue, + }, + ) + await expect(tx).revertedWith('Only tokens owner can call') + }) }) describe('rescueTokens', function () { it('should rescue tokens', async function () { From b76bb3efd1a8e287670d097a6b1276efbc0383e7 Mon Sep 17 00:00:00 2001 From: Miguel de Elias Date: Thu, 21 Sep 2023 13:36:31 +1000 Subject: [PATCH 4/6] chore: updated addresses after new deployment --- addresses.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/addresses.json b/addresses.json index 1a527e6..b8a16c2 100644 --- a/addresses.json +++ b/addresses.json @@ -4,14 +4,14 @@ "GraphToken": "0xc944E90C64B2c07662A292be6244BDf05Cda44a7", "L1GraphTokenGateway": "0x01cDC91B0A9bA741903aA3699BF4CE31d6C5cC06", "governor": "0x74Db79268e63302d3FC69FB5a7627F7454a41732", - "BillingConnector": "0xBa321653bDb2525c866d962aD66be358F6718d78" + "BillingConnector": "0x8017B9AF3F199CC6b08A48DA3859410F20bbea72" }, "5": { "ArbitrumInbox": "0x6BEbC4925716945D46F0Ec336D5C2564F419682C", "GraphToken": "0x5c946740441C12510a167B447B7dE565C20b9E3C", "L1GraphTokenGateway": "0xc82fF7b51c3e593D709BA3dE1b3a0d233D1DEca1", "governor": "0xc6B8Dd6163a0d6E3Ff37Eb4306DeD0069FE4bF59", - "BillingConnector": "0x1c133bF1A059F682910d9bf81a09931742ec7311" + "BillingConnector": "0x0de059845a3886A10d076A34d38b6b1aB24900b2" }, "137": { "Billing": "0x10829DB618E6F520Fa3A01c75bC6dDf8722fA9fE" From 93ee8c32f75785dec5cdaf3ef9c3676eadbf9a5f Mon Sep 17 00:00:00 2001 From: Miguel de Elias Date: Thu, 21 Sep 2023 13:37:05 +1000 Subject: [PATCH 5/6] fix: readme file with correct command for deploy/verify --- README.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index df55ae4..9ee27e2 100644 --- a/README.md +++ b/README.md @@ -87,10 +87,11 @@ Use mainnet (or goerli) to deploy BillingConnector: ``` hh deploy-billing-connector --network mainnet \ - --tokenGateway \ + --tokengateway \ --billing \ --token \ - --governor + --governor \ + --inbox ``` Then run this to verify on Etherscan: @@ -102,7 +103,8 @@ hh verify --network mainnet \ \ \ \ - + \ + ``` ### BanxaWrapper From 7856b914e8e8d55caeb265cd43f67d6a69fea0b0 Mon Sep 17 00:00:00 2001 From: Miguel de Elias Date: Thu, 12 Oct 2023 15:03:59 +1100 Subject: [PATCH 6/6] chore: bump version number --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 97d2f2b..5a510ad 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "graph-studio-billing-contracts", - "version": "1.0.0", + "version": "1.0.1", "description": "Graph Studio Billing Contracts", "main": "index.js", "scripts": {