Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 155 additions & 0 deletions contracts/script/SafePropose.s.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
// SPDX-License-Identifier: AGPL-3.0
pragma solidity ^0.8.0;

import { LibString } from "solady/utils/LibString.sol";
import { Surl } from "surl/Surl.sol";

import { BaseScript, stdJson } from "./Base.s.sol";

contract SafePropose is BaseScript {
using LibString for address;
using LibString for bytes;
using LibString for uint256;
using stdJson for string;
using Surl for string;

IMultiSendCallOnly internal constant MULTI_SEND = IMultiSendCallOnly(0x9641d764fc13c8B624c04430C7356C1C7C8102e2); // github.com/safe-global/safe-deployments v1.4.1
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Hardcoded MultiSendCallOnly address assumes deployment on all supported chains

The constant MULTI_SEND at line 16 uses address 0x9641d764fc13c8B624c04430C7356C1C7C8102e2 (Safe v1.4.1 MultiSendCallOnly). This is a deterministically deployed Safe contract that should exist on standard EVM chains. However, _chainPrefix() at line 119 includes opBNB (chain 204), which is a less common chain. If Safe's v1.4.1 MultiSendCallOnly is not deployed on opBNB, batch proposals (proposeBatch) would delegate-call to a non-existent contract. Single-call proposals would still work since they bypass MultiSend. Worth verifying the deployment exists on all six supported chains.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Explanatory comment on hardcoded address may violate no-comments convention

Line 16 has // github.com/safe-global/safe-deployments v1.4.1 documenting the provenance of the hardcoded MULTI_SEND address. The AGENTS.md rule states that the only allowed comments are static analysis annotations and TODO/HACK/FIXME markers. While this technically violates the rule, documenting the source of a hardcoded contract address is a reasonable safety practice — it allows future developers to verify the address. The existing codebase uses the // TODO remove after https://github.com/... pattern in contracts/test/Fork.t.sol:23-25 which is allowed because it uses the TODO prefix. A possible compromise: convert this to a TODO-style comment or encode the provenance into the variable name.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


function run(string calldata path) external {
string memory json = vm.readFile(path); // forge-lint: disable-line(unsafe-cheatcode)
uint256 length;
while (vm.keyExistsJson(json, string.concat(".transactions[", length.toString(), "]"))) ++length;
if (length == 0) revert EmptyBroadcast();
Call[] memory calls = new Call[](length);
address safe;
for (uint256 i = 0; i < length; ++i) {
string memory prefix = string.concat(".transactions[", i.toString(), "]");
if (keccak256(bytes(json.readString(string.concat(prefix, ".transactionType")))) != keccak256("CALL")) {
revert NotACall();
}
prefix = string.concat(prefix, ".transaction");
address from = json.readAddress(string.concat(prefix, ".from"));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The script assumes the from address in the input JSON is a Safe contract. It will fail if an EOA address is provided, as there's no validation.
Severity: MEDIUM

Suggested Fix

Add a check to validate that the safe address is a contract before attempting to call methods on it. This can be done by checking the address's code size, for example: if (safe.code.length == 0) revert NotASafeContract();. This will provide a clear error message to the user instead of a cryptic low-level failure.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: contracts/script/SafePropose.s.sol#L31

Potential issue: The `SafePropose.s.sol` script reads a `from` address from a broadcast
JSON file and assigns it to the `safe` variable. It then proceeds to call
contract-specific methods like `safe.nonce()` on this address. The script lacks any
validation to ensure the provided address is a contract and not an Externally Owned
Account (EOA). If a user mistakenly provides a standard broadcast file generated from an
EOA, the script will fail with a low-level error when it attempts to execute code on an
address that has none. This makes the script fragile and hard to debug for users.

Did we get this right? 👍 / 👎 to inform future reviews.

if (i == 0) safe = from;
else if (from != safe) revert SenderMismatch();
Comment on lines +31 to +33
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Read Safe address from explicit input, not tx sender

run currently treats .transactions[i].transaction.from as the Safe address, but in standard Forge broadcast artifacts that field is the broadcasting EOA (for example, existing contracts/broadcast/**/run-latest.json files use a deployer EOA in from). In that common workflow safe is set to an EOA, so subsequent safe.nonce() / getTransactionHash(...) calls in propose revert or build an invalid proposal, making the script unusable for normal broadcast outputs.

Useful? React with 👍 / 👎.

calls[i] = Call({
to: json.readAddress(string.concat(prefix, ".to")),
value: json.readUint(string.concat(prefix, ".value")),
data: json.readBytes(string.concat(prefix, ".input"))
});
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if (length == 1) propose(ISafe(safe), calls[0].to, calls[0].value, calls[0].data, 0);
else proposeBatch(ISafe(safe), calls);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

function proposeBatch(ISafe safe, Call[] memory calls) internal {
bytes memory packed;
for (uint256 i = 0; i < calls.length; ++i) {
packed = abi.encodePacked(packed, uint8(0), calls[i].to, calls[i].value, calls[i].data.length, calls[i].data);
}
propose(safe, address(MULTI_SEND), 0, abi.encodeCall(MULTI_SEND.multiSend, (packed)), 1);
}

function propose(ISafe safe, address to, uint256 value, bytes memory data, uint8 operation) internal virtual {
string memory hexSafe = address(safe).toHexStringChecksummed();
string memory url = string.concat(
"https://api.safe.global/tx-service/", _chainPrefix(), "/api/v2/safes/", hexSafe, "/multisig-transactions/"
);
Comment on lines +54 to +56
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate broadcast chain before building tx-service URL

run parses transactions from a broadcast file but never verifies that their .transaction.chainId matches the chain the script is currently connected to; propose always derives the endpoint from _chainPrefix() (current block.chainid). If someone accidentally feeds a run-latest.json from another network (easy in this repo because artifacts are stored per-chain), this can submit a proposal to the wrong Safe Transaction Service/network instead of failing fast. Add an explicit chain-id check before proposing.

Useful? React with 👍 / 👎.

Comment on lines +53 to +56
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Single-use hexSafe variable violates the inline extraction rule

hexSafe is consumed exactly once (line 55) yet is extracted into a named local variable on line 53. AGENTS.md's extraction rules state: "single-use = inline: a value consumed once stays at the point of consumption. … no exceptions for 'clarity' — the call site is already clear." The expression address(safe).toHexStringChecksummed() can be placed directly inside the string.concat call on line 55 without any technical issue.

Suggested change
string memory hexSafe = address(safe).toHexStringChecksummed();
string memory url = string.concat(
"https://api.safe.global/tx-service/", _chainPrefix(), "/api/v2/safes/", hexSafe, "/multisig-transactions/"
);
string memory url = string.concat(
"https://api.safe.global/tx-service/", _chainPrefix(), "/api/v2/safes/", address(safe).toHexStringChecksummed(), "/multisig-transactions/"
);
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

uint256 nonce = safe.nonce();
bytes32 safeTxHash =
safe.getTransactionHash(to, value, data, operation, 0, 0, 0, address(0), payable(address(0)), nonce);
Comment on lines +57 to +59
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use the next queued Safe nonce instead of safe.nonce()

When the target Safe already has a pending multisig transaction, safe.nonce() is still the last executed nonce, so this script will always propose a competing transaction for the current nonce instead of queueing the new action after the existing one. In that common multisig workflow, the generated contractTransactionHash is for the wrong slot and the proposal cannot be used as the next step in the queue.

Useful? React with 👍 / 👎.

address sender;
bytes memory signature;
{
(uint8 v, bytes32 r, bytes32 s) = vm.sign(safeTxHash);
sender = ecrecover(safeTxHash, v, r, s);
signature = abi.encodePacked(r, s, v);
}
Comment on lines +62 to +66
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider validating ecrecover result.

ecrecover returns address(0) if the signature or hash is malformed. While unlikely in normal operation, adding a guard would provide a clearer error message than a downstream API failure.

🛡️ Proposed fix
     {
       (uint8 v, bytes32 r, bytes32 s) = vm.sign(safeTxHash);
       sender = ecrecover(safeTxHash, v, r, s);
+      require(sender != address(0), "Invalid signature");
       signature = abi.encodePacked(r, s, v);
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{
(uint8 v, bytes32 r, bytes32 s) = vm.sign(safeTxHash);
sender = ecrecover(safeTxHash, v, r, s);
signature = abi.encodePacked(r, s, v);
}
{
(uint8 v, bytes32 r, bytes32 s) = vm.sign(safeTxHash);
sender = ecrecover(safeTxHash, v, r, s);
require(sender != address(0), "Invalid signature");
signature = abi.encodePacked(r, s, v);
}

string[] memory headers = new string[](1);
headers[0] = "Content-Type: application/json";
Comment on lines +67 to +68
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Add the required Safe API key header

When this posts to the hosted https://api.safe.global/tx-service/... endpoint, the request only includes Content-Type. Safe's current docs for the default Transaction Service say hosted requests should be authenticated with Authorization: Bearer ..., and their API-key guide notes that missing keys are rejected with 401 Unauthorized. In other words, on the official service this script will always revert with ProposalFailed instead of creating the proposal, because there is no way here to send the API key or point at a custom/self-hosted tx-service.

Useful? React with 👍 / 👎.

(uint256 status, bytes memory response) =
url.post(headers, _body(to, value, data, operation, nonce, safeTxHash, sender, signature));
if (status != 201) revert ProposalFailed(status, string(response));
}

// solhint-disable quotes
function _body(
address to,
uint256 value,
bytes memory data,
uint8 operation,
uint256 nonce,
bytes32 safeTxHash,
address sender,
bytes memory signature
) internal pure returns (string memory) {
return string.concat(
string.concat(
'{"to":"',
to.toHexStringChecksummed(),
'","value":"',
value.toString(),
'","data":"',
data.length == 0 ? "0x" : data.toHexString(),
'","operation":',
uint256(operation).toString(),
',"safeTxGas":"0","baseGas":"0","gasPrice":"0","gasToken":"',
address(0).toHexStringChecksummed(),
'","refundReceiver":"',
address(0).toHexStringChecksummed()
),
'","nonce":',
nonce.toString(),
',"contractTransactionHash":"',
bytes.concat(safeTxHash).toHexString(),
'","sender":"',
sender.toHexStringChecksummed(),
'","signature":"',
signature.toHexString(),
'"}'
);
Comment thread
itofarina marked this conversation as resolved.
}
// solhint-enable quotes

function _chainPrefix() internal view returns (string memory) {
if (block.chainid == 1) return "eth";
if (block.chainid == 10) return "oeth"; // cspell:ignore oeth
if (block.chainid == 56) return "bnb";
if (block.chainid == 137) return "pol";
if (block.chainid == 204) return "opbnb"; // cspell:ignore opbnb
if (block.chainid == 8453) return "base";
if (block.chainid == 42_161) return "arb1";
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The script uses the wrong Safe Transaction Service endpoint for opBNB. It points to api.safe.global instead of the correct bnbchain.org domain, causing proposals to fail.
Severity: HIGH

Suggested Fix

Update the script to use the correct endpoint for opBNB. Instead of constructing the URL with the api.safe.global domain for chain ID 204, it should use https://safe-transaction-opbnb-mainnet.bnbchain.org/. This may require special handling for the opBNB chain ID to use a different base URL.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: contracts/script/SafePropose.s.sol#L120

Potential issue: The `SafePropose` script adds support for opBNB (chain ID 204) by
constructing a URL to the Safe Transaction Service. However, it incorrectly uses the
generic `api.safe.global` domain. opBNB has its own dedicated Safe Transaction Service
infrastructure hosted at `safe-transaction-opbnb-mainnet.bnbchain.org`. Consequently,
any attempt to propose a transaction on opBNB will target a non-existent endpoint,
receive a non-201 HTTP status, and cause the script to revert with a `ProposalFailed`
error. This renders the opBNB proposal feature non-functional.

revert UnsupportedChain();
Comment on lines +113 to +121
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Add chain prefixes for the repo's supported testnets

_chainPrefix() only recognizes mainnet chain IDs, but this repository already ships contract configs and broadcast artifacts for Base Sepolia (84532) and OP Sepolia (11155420) in contracts/deploy.json and contracts/broadcast/**. Running the new script on either staging network will always revert with UnsupportedChain(), which makes the tool unusable for the existing non-production deployment flow.

Useful? React with 👍 / 👎.

Comment on lines +120 to +121
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Map chain id 56 before reverting UnsupportedChain

SafePropose now has companion config for BNB (bnb_smart_chain in contracts/foundry.toml and chain "56" data in contracts/deploy.json), but _chainPrefix() never handles block.chainid == 56, so the script always reverts with UnsupportedChain() on BNB instead of proposing to Safe’s tx-service. This makes the new BNB wiring unusable in practice whenever operators run the script against that network.

Useful? React with 👍 / 👎.

}
Comment thread
itofarina marked this conversation as resolved.
Comment on lines +113 to +122
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Safe API v2 URL format and chain prefixes should be verified

The URL pattern https://api.safe.global/tx-service/{prefix}/api/v2/safes/{safe}/multisig-transactions/ and the chain prefix mapping (eth=1, oeth=10, pol=137, base=8453, arb1=42161) at contracts/script/SafePropose.s.sol:113-120 correspond to the Safe Transaction Service's newer API format. These prefixes are not easily verifiable from the codebase alone. If Safe changes their API URL structure or chain prefixes, this would silently fail with a non-201 status (caught by the ProposalFailed revert). Worth verifying against current Safe API documentation.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Comment thread
sentry[bot] marked this conversation as resolved.
}

error EmptyBroadcast();
error NotACall();
error ProposalFailed(uint256 status, string response);
error SenderMismatch();
error UnsupportedChain();

struct Call {
address to;
uint256 value;
bytes data;
}

interface IMultiSendCallOnly {
function multiSend(bytes memory transactions) external;
}

interface ISafe {
function nonce() external view returns (uint256);
function getTransactionHash(
address to,
uint256 value,
bytes calldata data,
uint8 operation,
uint256 safeTxGas,
uint256 baseGas,
uint256 gasPrice,
address gasToken,
address payable refundReceiver,
uint256 _nonce
) external view returns (bytes32);
}
Loading