Sayfer audit fix

[sirpy]

Description

[sourcery-ai]

Hey - I've found 4 issues, and left some high level feedback:

• In IdentityV3.blacklist, decrementing whitelistedCount unconditionally can underflow or desync the counter if the account is not currently counted as whitelisted (e.g., already blacklisted or never whitelisted); consider checking the prior status or tracking the count via a dedicated helper to avoid reverting or miscounting.
• The new nonReentrant logic in GenericDistributionHelper uses a raw _status flag; for clarity and to avoid future misuse, consider using named constants (e.g., _NOT_ENTERED, _ENTERED) and initializing _status explicitly in the constructor or initialize to match the intended invariant.
• The four upgrade*Fix functions in gip-25-xdc-upgrade-ubi.ts share a large amount of boilerplate (signer/guardian resolution, Safe execution setup, logging); factoring this into a shared helper would reduce duplication and the risk of inconsistencies between networks.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `IdentityV3.blacklist`, decrementing `whitelistedCount` unconditionally can underflow or desync the counter if the account is not currently counted as whitelisted (e.g., already blacklisted or never whitelisted); consider checking the prior status or tracking the count via a dedicated helper to avoid reverting or miscounting.
- The new `nonReentrant` logic in `GenericDistributionHelper` uses a raw `_status` flag; for clarity and to avoid future misuse, consider using named constants (e.g., `_NOT_ENTERED`, `_ENTERED`) and initializing `_status` explicitly in the constructor or `initialize` to match the intended invariant.
- The four `upgrade*Fix` functions in `gip-25-xdc-upgrade-ubi.ts` share a large amount of boilerplate (signer/guardian resolution, Safe execution setup, logging); factoring this into a shared helper would reduce duplication and the risk of inconsistencies between networks.

## Individual Comments

### Comment 1
<location> `scripts/multichain-deploy/helpers.ts:360` </location>
<code_context>
   functionInputs,
   safeAddress: string,
   safeSignerOrNetwork?: Signer | string,
+  safeOptions: SafeTransactionOptionalProps = {},
   isSimulation = false
 ) => {
</code_context>

<issue_to_address>
**issue (bug_risk):** The new executeViaSafe parameter order is a breaking change and may silently mis-route existing call-site arguments.

By inserting `safeOptions` before `isSimulation`, the call signature from existing callers changes. A pre-existing call like `executeViaSafe(contracts, values, sigs, inputs, safeAddress, networkOrSigner, isSimulation)` will now interpret `networkOrSigner` as `safeSignerOrNetwork`, `isSimulation` as `safeOptions`, and use the default for `isSimulation`. This can result in `safeOptions` being a boolean and `isSimulation` always `false`, causing subtle misbehavior. To avoid this, either append `safeOptions` to the end of the parameter list or update all call sites and consider a temporary overload/deprecation path for the old signature.
</issue_to_address>

### Comment 2
<location> `contracts/identity/IdentityV3.sol:241` </location>
<code_context>
 		address account
 	) public onlyRole(IDENTITY_ADMIN_ROLE) whenNotPaused {
 		identities[account].status = 255;
+		whitelistedCount -= 1;
 		emit BlacklistAdded(account);
 	}
</code_context>

<issue_to_address>
**issue (bug_risk):** Blindly decrementing whitelistedCount risks underflow / incorrect counts if blacklistIdentity is called on non-whitelisted addresses.

This relies on `blacklistIdentity` being called only once per address and only for currently-whitelisted addresses. In Solidity 0.8, calling it on an already-blacklisted or never-whitelisted address will revert due to underflow, which may be undesirable for admin flows. Consider updating `whitelistedCount` only on actual status transitions (e.g., when the previous status was whitelisted) and decide whether contract addresses tracked in `whitelistedContracts` need separate handling to keep both counters aligned.
</issue_to_address>

### Comment 3
<location> `scripts/proposals/gip-25-xdc-upgrade-ubi.ts:743` </location>
<code_context>
-main().catch(console.log);
+// upgradeCeloFix("production-celo", false).catch(console.log);
+// upgradeEthFix("production-mainnet", false).catch(console.log);
+upgradeFuseFix("production", false).catch(console.log);
+// upgradeXdcFix("production-xdc", false).catch(console.log);
+// verifyUpgradeCeloStep1("production-celo");
</code_context>

<issue_to_address>
**issue (bug_risk):** Hard-coding upgradeFuseFix as the default entrypoint may cause accidental mainnet execution if the script is run as-is.

The script used to call `main()`, but now directly calls `upgradeFuseFix("production", false)` with other calls and `main()` commented out. Anyone running this script expecting the previous generic behavior will instead trigger a production Fuse upgrade. To reduce this operational risk, keep `main()` as the default entrypoint and route to `upgradeFuseFix` via flags or environment checks, or revert this change after the one-off operation is complete.
</issue_to_address>

### Comment 4
<location> `scripts/proposals/gip-25-xdc-upgrade-ubi.ts:183` </location>
<code_context>
     console.log("UBI claim from connected account tx:", claimTx.events);
   }
 };
+export const upgradeCeloFix = async (network, checksOnly) => {
+  let [root] = await ethers.getSigners();
+
</code_context>

<issue_to_address>
**issue (complexity):** Consider extracting the shared orchestration logic from the various `upgrade{Celo,Xdc,Fuse,Eth}Fix` functions into a single reusable helper that each network-specific function calls with its unique configuration.

You can collapse almost all of the `upgrade{Celo,Xdc,Fuse,Eth}Fix` duplication into a single helper while keeping behavior identical.

The common pattern is:

- signer / guardian / `isProduction` / `isSimulation` / `networkEnv` / `release` setup  
- optional impersonation + funding of `GuardiansSafe`  
- compute `bridgeImpl` and build `proposalActions`  
- map actions → `{ contracts, signatures, inputs, values }`  
- `if (isProduction && !checksOnly) executeViaSafe(...) else if (!checksOnly) executeViaGuardian(...)`  
- optional post‑checks (ETH‑specific, and potentially others later)

You can factor that into a generic helper and pass only the differences from each fix function.

### Example helper

```ts
type ProposalAction = [string, string, string, string];

async function runBridgeUpgradeFix({
  networkArg,
  checksOnly,
  safeNetworkName,
  safeOptions,
  buildActions,
  postChecks
}: {
  networkArg: string;
  checksOnly: boolean;
  safeNetworkName: string;
  safeOptions?: { nonce?: number };
  buildActions: (ctx: {
    bridgeImpl: string;
    release: { [key: string]: any };
  }) => ProposalAction[];
  postChecks?: (ctx: {
    networkEnv: string;
    isProduction: boolean;
    isSimulation: boolean;
    release: { [key: string]: any };
    guardian: ethers.Signer;
  }) => Promise<void>;
}) {
  let [root] = await ethers.getSigners();
  const isProduction = networkName.includes("production");
  if (isProduction) verifyProductionSigner(root);

  let networkEnv = networkName;
  let guardian: ethers.Signer = root;

  if (isSimulation) {
    networkEnv = networkArg;
  }

  let release: { [key: string]: any } = dao[networkEnv];

  console.log("signer:", root.address, { networkEnv, isSimulation, isProduction, release });

  if (isSimulation) {
    networkEnv = networkArg;
    guardian = await ethers.getImpersonatedSigner(release.GuardiansSafe);

    await root.sendTransaction({
      value: ethers.utils.parseEther("1"),
      to: release.GuardiansSafe
    });
  }

  const bridgeImpl = bridgeUpgradeImpl[networkEnv];
  const proposalActions = buildActions({ bridgeImpl, release });

  console.log({
    networkEnv,
    guardian: await guardian.getAddress(),
    isSimulation,
    isProduction,
    release
  });

  const proposalContracts = proposalActions.map(a => a[0]);
  const proposalFunctionSignatures = proposalActions.map(a => a[1]);
  const proposalFunctionInputs = proposalActions.map(a => a[2]);
  const proposalEthValues = proposalActions.map(a => a[3]);

  if (isProduction && !checksOnly) {
    await executeViaSafe(
      proposalContracts,
      proposalEthValues,
      proposalFunctionSignatures,
      proposalFunctionInputs,
      release.GuardiansSafe,
      safeNetworkName,
      safeOptions
    );
  } else if (!checksOnly) {
    await executeViaGuardian(
      proposalContracts,
      proposalEthValues,
      proposalFunctionSignatures,
      proposalFunctionInputs,
      guardian,
      networkEnv
    );
  }

  if (postChecks) {
    await postChecks({ networkEnv, isProduction, isSimulation, release, guardian });
  }
}
```

### Network‑specific functions become thin wrappers

**Celo:**

```ts
export const upgradeCeloFix = async (networkArg: string, checksOnly: boolean) => {
  const toBurn = "5515965554075495700269228267";

  await runBridgeUpgradeFix({
    networkArg,
    checksOnly,
    safeNetworkName: "celo",
    safeOptions: { nonce: 5 },
    buildActions: ({ bridgeImpl, release }) => [
      [
        release.MpbBridge,
        "upgradeTo(address)",
        ethers.utils.defaultAbiCoder.encode(["address"], [bridgeImpl]),
        "0"
      ],
      [
        release.GoodDollar,
        "burn(uint256)",
        ethers.utils.defaultAbiCoder.encode(["uint256"], [toBurn.toString()]),
        "0"
      ]
    ]
  });
};
```

**XDC:**

```ts
export const upgradeXdcFix = async (networkArg: string, checksOnly: boolean) => {
  await runBridgeUpgradeFix({
    networkArg,
    checksOnly,
    safeNetworkName: "xdc",
    buildActions: ({ bridgeImpl, release }) => [
      [
        release.MpbBridge,
        "upgradeTo(address)",
        ethers.utils.defaultAbiCoder.encode(["address"], [bridgeImpl]),
        "0"
      ]
    ]
  });
};
```

**Fuse:**

```ts
export const upgradeFuseFix = async (networkArg: string, checksOnly: boolean) => {
  await runBridgeUpgradeFix({
    networkArg,
    checksOnly,
    safeNetworkName: "fuse",
    buildActions: ({ bridgeImpl, release }) => [
      [
        release.MpbBridge,
        "upgradeTo(address)",
        ethers.utils.defaultAbiCoder.encode(["address"], [bridgeImpl]),
        "0"
      ]
    ]
  });
};
```

**ETH / mainnet (with post‑checks):**

```ts
export const upgradeEthFix = async (networkArg: string, checksOnly: boolean) => {
  await runBridgeUpgradeFix({
    networkArg,
    checksOnly,
    safeNetworkName: "mainnet",
    safeOptions: { nonce: 15 },
    buildActions: ({ bridgeImpl, release }) => {
      const upgradeCall = ethers.utils
        .keccak256(ethers.utils.toUtf8Bytes("upgrade()"))
        .substring(0, 10);

      return [
        [
          release.MpbBridge,
          "upgradeToAndCall(address,bytes)",
          ethers.utils.defaultAbiCoder.encode(["address", "bytes"], [bridgeImpl, upgradeCall]),
          "0"
        ]
      ];
    },
    postChecks: async ({ networkEnv, isProduction, isSimulation, release }) => {
      if (isSimulation || !isProduction) {
        const ctrl = (await ethers.getContractAt("Controller", release.Controller)) as Controller;
        const isMinterScheme = await ctrl.getSchemePermissions(release.MpbBridge, release.Avatar);
        console.log("Bridge minter permissions on avatar:", isMinterScheme);

        const mpb = (await ethers.getContractAt("IMessagePassingBridge", release.MpbBridge)) as IMessagePassingBridge;
        console.log("xdc lz chainid:", await mpb.toLzChainId(50));
      }
    }
  });
};
```

This keeps all existing behaviors (same actions, safe network strings, nonce overrides, and ETH‑specific verification) but centralizes the orchestration logic. Any future change to signer selection, simulation behavior, logging, or execution routing only needs to be done in one place, and the per‑network functions clearly show just what actually varies.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[openzeppelin-code]

Sayfer audit fix

Generated at commit: 89787e45ae8bb039fdb9a99bb58cd63d9b656e64

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	3 5 0 16 44 68
Dependencies	Critical High Medium Low Note Total	0 0 1 0 130 131

---

For more details view the full report in OpenZeppelin Code Inspector