Skip to content

Invalid SAFE_NONCE values are silently treated as missing #443

Description

@Mabolla

Summary

MultisigScript._getNonce treats a missing nonce override and an invalid nonce override the same way.

Both SAFE_NONCE and SAFE_NONCE_{UPPERCASE_SAFE_ADDRESS} are read through vm.envUint inside empty try/catch blocks. Foundry’s envUint reverts both when the variable is absent and when its value cannot be parsed. Because both errors are swallowed, a configured but malformed override is silently ignored.

Current behavior

The current implementation is:

try vm.envUint({ name: "SAFE_NONCE" }) {
    nonce = vm.envUint({ name: "SAFE_NONCE" });
} catch { }

string memory envVarName =
    string.concat("SAFE_NONCE_", vm.toUppercase({ input: vm.toString({ value: safe }) }));

try vm.envUint({ name: envVarName }) {
    nonce = vm.envUint({ name: envVarName });
} catch { }

For example, if SAFE_NONCE=abc is present, vm.envUint("SAFE_NONCE") cannot parse it and reverts. The empty catch suppresses that error, so _getNonce continues with the Safe’s current on-chain nonce unless a valid address-specific override is present.

There is no warning when the malformed override is ignored because the existing log only runs when the final nonce differs from the on-chain nonce.

Why this matters

The comments describe nonce overrides as a way to pre-sign transactions that may require safe.nonce + 1. Silently falling back to the current nonce can therefore produce transaction data different from what the operator intended to sign.

A missing optional override should fall back to the on-chain nonce. A present but invalid override should fail clearly instead of being treated as missing.

Expected behavior

  • If the address-specific nonce variable exists, parse and use it. If it is invalid, preserve the parsing error.
  • Otherwise, if SAFE_NONCE exists, parse and use it. If it is invalid, preserve the parsing error.
  • Fall back to the Safe’s current nonce only when neither variable exists.

Checking the address-specific variable first also matches the precedence documented immediately above _getNonce.

Suggested implementation

Use vm.envExists to distinguish absence from invalid contents while preserving address-specific precedence:

string memory envVarName =
    string.concat("SAFE_NONCE_", vm.toUppercase({ input: vm.toString({ value: safe }) }));

if (vm.envExists(envVarName)) {
    return vm.envUint(envVarName);
}

if (vm.envExists("SAFE_NONCE")) {
    return vm.envUint("SAFE_NONCE");
}

return IGnosisSafe(safe).nonce();

The exact structure can follow the repository’s preferred style.

Regression coverage

Add focused tests covering:

  1. neither variable exists: use the on-chain nonce;
  2. valid SAFE_NONCE: use the general override;
  3. valid address-specific override: it takes precedence over the general override;
  4. malformed selected override: revert instead of silently falling back;
  5. malformed general override shadowed by a valid address-specific override: use the valid address-specific value.

I’m ready to prepare the PR and regression tests if this behavior is confirmed as the intended direction.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions