Merge pull request #144 from OffchainLabs/audit-ci-dev

add audit ci; update / allowlist dependencies; update waffle tests for v4 [v2]
OffchainLabs · Feb 21, 2024 · b8a12f1 · b8a12f1
2 parents 7fbbdd4 + 2e5a2cb
commit b8a12f1
Show file tree

Hide file tree

Showing 7 changed files with 1,215 additions and 4,724 deletions.
diff --git a/.github/workflows/audit-ci.yml b/.github/workflows/audit-ci.yml
@@ -0,0 +1,48 @@
+name: Audit NPM packages
+
+on:
+  workflow_dispatch:
+  pull_request:
+  merge_group:
+  push:
+    branches:
+      - main
+      - develop
+
+jobs:
+  install:
+    name: 'Install'
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        node-version: [16, 18, 20]
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v3
+
+      - name: Set up Node.js
+        uses: actions/setup-node@v3
+        with:
+          node-version: ${{ matrix.node-version }}
+
+      - name: Install node_modules
+        uses: OffchainLabs/actions/node-modules/install@main
+
+  yarn-audit:
+    name: Audit
+    runs-on: ubuntu-latest
+    needs: install
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v3
+
+      - name: Set up Node.js
+        uses: actions/setup-node@v3
+        with:
+          node-version: ${{ matrix.node-version }}
+
+      - name: Restore node_modules
+        uses: OffchainLabs/actions/node-modules/restore@main
+
+      - name: Run audit
+        run: yarn audit:ci
diff --git a/audit-ci.jsonc b/audit-ci.jsonc
@@ -0,0 +1,56 @@
+{
+  "$schema": "https://github.com/IBM/audit-ci/raw/main/docs/schema.json",
+  "low": true,
+  "allowlist": [
+    // OpenZeppelin Contracts's SignatureChecker may revert on invalid EIP-1271 signers
+    "GHSA-4g63-c64m-25w9",
+    // OpenZeppelin Contracts's GovernorVotesQuorumFraction updates to quorum may affect past defeated proposals
+    "GHSA-xrc4-737v-9q75",
+    // OpenZeppelin Contracts's ERC165Checker may revert instead of returning false
+    "GHSA-qh9x-gcfh-pcrw",
+    // OpenZeppelin Contracts vulnerable to ECDSA signature malleability. Only an issue for the functions that take a single `bytes` argument, and not the functions that take `r, v, s` or `r, vs` as separate arguments.
+    "GHSA-4h98-2769-gh6h",
+    // GovernorCompatibilityBravo may trim proposal calldata
+    "GHSA-93hq-5wgc-jc82",
+    // OpenZeppelin Contracts ERC165Checker unbounded gas consumption
+    "GHSA-7grf-83vw-6f5x",
+    // OpenZeppelin: Using ERC2771Context with a custom forwarder can yield address(0)
+    "GHSA-g4vp-m682-qqmp",
+    // OpenZeppelin Contracts TransparentUpgradeableProxy clashing selector calls may not be delegated
+    "GHSA-mx2q-35m2-x2rh",
+    // OpenZeppelin Contracts's governor proposal creation may be blocked by frontrunning
+    "GHSA-5h3x-9wvq-w4m2",
+    // axios cookies data-privacy issue; used only in hardhat-deploy and sol2uml (dev deps)
+    "GHSA-wf5p-g6vw-rhxx",
+    // semver vulnerable to Regular Expression Denial of Service
+    "GHSA-c2qf-rxjj-qqgw",
+    // flat vulnerable to Prototype Pollution
+    "GHSA-2j2x-2gpw-g8fm",
+    // regular expression DoS in debug
+    "GHSA-gxpj-cx7g-858c",
+    // tough-cookie Prototype Pollution vulnerability; used only via eth-gas-reporter
+    "GHSA-72xf-g2v4-qvf3",
+    // minimatch ReDoS vulnerability
+    "GHSA-f8q6-p94x-37v3",
+    // Server-Side Request Forgery in Request
+    "GHSA-p8p7-x288-28g6",
+    // Prototype Pollution in lodash
+    "GHSA-p6mc-m468-83gw",
+    //  OpenZeppelin Contracts using MerkleProof multiproofs may allow proving arbitrary leaves for specific trees; unused
+    "GHSA-wprv-93r4-jj2p",
+    //  follow-redirects improperly handles URLs in the url.parse() function
+    "GHSA-jchw-25xp-jwwc",
+    // Undici's cookie header not cleared on cross-origin redirect in fetch,
+    "GHSA-wqq4-5wpv-mx2g",
+    // yargs-parser Vulnerable to Prototype Pollution
+    "GHSA-p9pc-299p-vxgp",
+    // Axios vulnerable to Server-Side Request Forgery
+    "GHSA-4w2v-q235-vp99",
+    // axios Inefficient Regular Expression Complexity vulnerability
+    "GHSA-cph5-m8f7-6c5x",
+    // Exposure of Sensitive Information to an Unauthorized Actor in follow-redirects
+    "GHSA-pw2r-vq6v-hr8c",
+    // Exposure of sensitive information in follow-redirects
+    "GHSA-74fj-2j2h-c42q"
+  ]
+}
diff --git a/package.json b/package.json
@@ -18,6 +18,8 @@
     "url": "https://github.com/offchainlabs/nitro-contracts/issues"
   },
   "scripts": {
+    "audit:ci": "audit-ci --config ./audit-ci.jsonc",
+    "audit:fix": "yarn-audit-fix",
     "prepublishOnly": "hardhat clean && forge clean && hardhat compile && yarn build:forge:yul",
     "build:all": "yarn build && yarn build:forge",
     "build": "hardhat compile",
@@ -63,14 +65,15 @@
     "@typescript-eslint/eslint-plugin": "^5.14.0",
     "@typescript-eslint/eslint-plugin-tslint": "^5.27.1",
     "@typescript-eslint/parser": "^5.14.0",
+    "audit-ci": "^6.6.1",
     "chai": "^4.3.4",
     "dotenv": "^16.3.1",
     "eslint": "^8.23.1",
     "eslint-config-prettier": "^8.3.0",
     "eslint-plugin-mocha": "^9.0.0",
     "eslint-plugin-prettier": "^4.0.0",
-    "ethereum-waffle": "^3.4.0",
-    "ethers": "^5.5.2",
+    "ethereum-waffle": "^4.0.10",
+    "ethers": "^5.5.4",
     "hardhat": "^2.17.2",
     "hardhat-deploy": "^0.11.37",
     "hardhat-gas-reporter": "^1.0.9",
@@ -83,7 +86,8 @@
     "solidity-coverage": "^0.8.4",
     "ts-node": "^10.4.0",
     "tslint": "^6.1.3",
-    "typechain": "^8.0.0",
-    "typescript": "^4.5.4"
+    "typechain": "^8.3.2",
+    "typescript": "^4.5.4",
+    "yarn-audit-fix": "^10.0.7"
   }
 }
diff --git a/test/contract/arbRollup.spec.ts b/test/contract/arbRollup.spec.ts
@@ -1397,11 +1397,9 @@ describe('ArbRollup', () => {
   it('can set is sequencer', async function () {
     const testAddress = await accounts[9].getAddress()
     expect(await sequencerInbox.isSequencer(testAddress)).to.be.false
-    await expect(
-      sequencerInbox.setIsSequencer(testAddress, true)
-    ).to.revertedWith(
-      `NotBatchPosterManager("${await sequencerInbox.signer.getAddress()}")`
-    )
+    await expect(sequencerInbox.setIsSequencer(testAddress, true))
+      .to.revertedWith(`NotBatchPosterManager`)
+      .withArgs(await sequencerInbox.signer.getAddress())
     expect(await sequencerInbox.isSequencer(testAddress)).to.be.false
 
     await (
@@ -1424,11 +1422,9 @@ describe('ArbRollup', () => {
   it('can set a batch poster', async function () {
     const testAddress = await accounts[9].getAddress()
     expect(await sequencerInbox.isBatchPoster(testAddress)).to.be.false
-    await expect(
-      sequencerInbox.setIsBatchPoster(testAddress, true)
-    ).to.revertedWith(
-      `NotBatchPosterManager("${await sequencerInbox.signer.getAddress()}")`
-    )
+    await expect(sequencerInbox.setIsBatchPoster(testAddress, true))
+      .to.revertedWith(`NotBatchPosterManager`)
+      .withArgs(await sequencerInbox.signer.getAddress())
     expect(await sequencerInbox.isBatchPoster(testAddress)).to.be.false
 
     await (
@@ -1455,7 +1451,9 @@ describe('ArbRollup', () => {
     )
     await expect(
       sequencerInbox.connect(accounts[8]).setBatchPosterManager(testManager)
-    ).to.revertedWith(`NotOwner("${testManager}", "${upgradeExecutor}")`)
+    )
+      .to.revertedWith('NotOwner')
+      .withArgs(testManager, upgradeExecutor)
     expect(await sequencerInbox.batchPosterManager()).to.eq(
       await batchPosterManager.getAddress()
     )
@@ -1471,7 +1469,7 @@ describe('ArbRollup', () => {
 
   it('should fail the chainid fork check', async function () {
     await expect(sequencerInbox.removeDelayAfterFork()).to.revertedWith(
-      'NotForked()'
+      'NotForked'
     )
   })
 
@@ -1485,7 +1483,7 @@ describe('ArbRollup', () => {
         0,
         0
       )
-    ).to.revertedWith('NotBatchPoster()')
+    ).to.revertedWith('NotBatchPoster')
   })
 
   it('should fail the onlyValidator check', async function () {

diff --git a/test/contract/sequencerInboxForceInclude.spec.ts b/test/contract/sequencerInboxForceInclude.spec.ts
@@ -196,9 +196,7 @@ describe('SequencerInboxForceInclude', async () => {
       messageDataHash
     )
     if (expectedErrorType) {
-      await expect(forceInclusionTx).to.be.revertedWith(
-        `reverted with custom error '${expectedErrorType}()'`
-      )
+      await expect(forceInclusionTx).to.be.revertedWith(expectedErrorType)
     } else {
       await (await forceInclusionTx).wait()
 
@@ -621,7 +619,7 @@ describe('SequencerInboxForceInclude', async () => {
         ethers.constants.AddressZero,
         '0x'
       )
-    ).to.revertedWith('NotForked()')
+    ).to.revertedWith('NotForked')
   })
 
   it('should fail to call sendUnsignedTransactionToFork', async function () {
@@ -635,14 +633,14 @@ describe('SequencerInboxForceInclude', async () => {
         0,
         '0x'
       )
-    ).to.revertedWith('NotForked()')
+    ).to.revertedWith('NotForked')
   })
 
   it('should fail to call sendWithdrawEthToFork', async function () {
     const { inbox } = await setupSequencerInbox()
     await expect(
       inbox.sendWithdrawEthToFork(0, 0, 0, 0, ethers.constants.AddressZero)
-    ).to.revertedWith('NotForked()')
+    ).to.revertedWith('NotForked')
   })
 
   it('can upgrade Inbox', async () => {

diff --git a/test/contract/validatorWallet.spec.ts b/test/contract/validatorWallet.spec.ts
@@ -101,11 +101,13 @@ describe('Validator Wallet', () => {
 
     await expect(
       wallet.connect(executor).executeTransaction(data, rollupMock1.address, 0)
-    ).to.be.revertedWith(
-      `OnlyOwnerDestination("${await owner.getAddress()}", "${await executor.getAddress()}", "${
-        rollupMock1.address
-      }")`
     )
+      .to.be.revertedWith('OnlyOwnerDestination')
+      .withArgs(
+        `${await owner.getAddress()}`,
+        `${await executor.getAddress()}`,
+        `${rollupMock1.address}`
+      )
     await expect(
       wallet.connect(owner).executeTransaction(data, rollupMock1.address, 0)
     ).to.emit(rollupMock1, 'WithdrawTriggered')
@@ -135,10 +137,12 @@ describe('Validator Wallet', () => {
           [rollupMock1.address, rollupMock2.address],
           [0, 0]
         )
-    ).to.be.revertedWith(
-      `OnlyOwnerDestination("${await owner.getAddress()}", "${await executor.getAddress()}", "${
-        rollupMock2.address
-      }")`
     )
+      .to.be.revertedWith('OnlyOwnerDestination')
+      .withArgs(
+        `${await owner.getAddress()}`,
+        `${await executor.getAddress()}`,
+        `${rollupMock2.address}`
+      )
   })
 })