Skip to content
Open
Show file tree
Hide file tree
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
154 changes: 154 additions & 0 deletions contracts/script/SafePropose.s.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
// 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 MULTISEND = IMultiSendCallOnly(0x9641d764fc13c8B624c04430C7356C1C7C8102e2); // github.com/safe-global/safe-deployments v1.4.1

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"));
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"))
});
}
if (length == 1) propose(ISafe(safe), calls[0].to, calls[0].value, calls[0].data, 0);
else proposeBatch(ISafe(safe), calls);
}

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(MULTISEND), 0, abi.encodeCall(MULTISEND.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 👍 / 👎.

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(),
'"}'
);
}
// solhint-enable quotes

function _chainPrefix() internal view returns (string memory) {
if (block.chainid == 1) return "eth";
if (block.chainid == 10) return "oeth";
if (block.chainid == 137) return "pol";
if (block.chainid == 8453) return "base";
if (block.chainid == 42_161) return "arb1";
if (block.chainid == 204) return "opbnb";
revert UnsupportedChain();
Comment on lines +113 to +120
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 👍 / 👎.

}
}

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);
}
2 changes: 2 additions & 0 deletions cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,13 @@
"modelcontextprotocol",
"moti",
"multicall",
"MULTISEND",
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.

🔴 One-off identifiers added to global cspell dictionary instead of using inline cspell:ignore

MULTISEND (a Solidity constant name) and oeth (a chain prefix string literal) each appear only in contracts/script/SafePropose.s.sol and nowhere else in the codebase. Per AGENTS.md: "only add words to cspell.json when the term is a real project-relevant word that appears broadly (e.g., a protocol name, a library, a domain term)... one-off occurrences (variable names, company names, urls, hashes, identifiers) stay as inline cspell:ignore — never pollute the global dictionary with them." Solidity supports // comments, so both can use inline cspell:ignore annotations on the lines where they appear.

Open in Devin Review

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

"mysten",
"natspec",
"nfmelendez",
"nomiclabs",
"nystrom",
"oeth",
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.

🟡 One-off chain prefix "oeth" added to global cspell dictionary instead of inline cspell:ignore

"oeth" is a Safe Transaction Service chain prefix string that only appears in contracts/script/SafePropose.s.sol:115. AGENTS.md states: "one-off occurrences (variable names, company names, urls, hashes, identifiers) stay as inline cspell:ignore — never pollute the global dictionary with them." This is a one-off identifier (Optimism's chain prefix abbreviation), not a broadly-used project domain term. An inline // cspell:ignore oeth annotation on contracts/script/SafePropose.s.sol:115 is the correct approach.

Open in Devin Review

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

Comment on lines +107 to +113
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Add "opbnb" to the words list for consistency.

Static analysis flags opbnb (opBNB chain prefix used in SafePropose.s.sol line 119) as an unknown word. Since oeth was added as a chain prefix, opbnb should be included as well for consistency.

🔧 Proposed fix
     "oeth",
     "offramp",
+    "opbnb",
     "oneline",

"offramp",
"oneline",
"onesignal",
Expand Down