Merge pull request from GHSA-5wm7-m7p7-hp2r

fix: denial of service attack on addToL2WithPermit function

README.md

@@ -87,10 +87,11 @@ Use mainnet (or goerli) to deploy BillingConnector:
 
 ```
 hh deploy-billing-connector --network mainnet \
- --tokenGateway <L1GRAPHTOKENGATEWAY_ADDRESS> \
+ --tokengateway <L1GRAPHTOKENGATEWAY_ADDRESS> \
  --billing <L2_BILLING_ADDRESS> \
  --token <GRT_ADDRESS> \
- --governor <GOVERNOR_ADDRESS>
+ --governor <GOVERNOR_ADDRESS> \
+ --inbox <ARBITRUM_INBOX_ADDRESS>
 ```
 
 Then run this to verify on Etherscan:
@@ -102,7 +103,8 @@ hh verify --network mainnet \
  <L2_BILLING_ADDRESS> \
  <GRT_ADDRESS> \
  <GOVERNOR_ADDRESS> \
- <L2GRAPHTOKENGATEWAY_ADDRESS>
+ <L2GRAPHTOKENGATEWAY_ADDRESS> \
+ <ARBITRUM_INBOX_ADDRESS>
 ```
 
 ### BanxaWrapper

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"

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,7 +179,8 @@ 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);
+ 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);
  }
 
@@ -261,4 +263,38 @@ 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 {
+ IERC20WithPermit token = IERC20WithPermit(address(graphToken));
+ // Try permit() before allowance check to advance nonce if possible
+ try token.permit(_owner, _spender, _value, _deadline, _v, _r, _s) {
+ return;
+ } catch Error(string memory reason) {
+ // Check for existing allowance before reverting
+ if (token.allowance(_owner, _spender) >= _value) {
+ return;
+ }
+
+ revert(reason);
+ }
+ }
 }

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": {

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,
@@ -341,13 +341,93 @@ 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)
+
+ // 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 with permit
+ const tx = await billingConnector
+ .connect(me.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]),
+ ])
+
+ // 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)
  const signedPermit = createSignedPermit(permit, myPrivateKey, SALT)
 
  const tx = billingConnector
- .connect(user1.signer)
+ .connect(me.signer)
  .addToL2WithPermit(
  me.address,
  permit.value,
@@ -369,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,
@@ -391,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,
@@ -413,7 +493,7 @@ describe('BillingConnector', () => {
  const signedPermit = createSignedPermit(permit, myPrivateKey, SALT)
 
  const tx = billingConnector
- .connect(user1.signer)
+ .connect(me.signer)
  .addToL2WithPermit(
  AddressZero,
  permit.value,
@@ -435,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,
@@ -457,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,
@@ -474,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 () {