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
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 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.


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"))
});
}
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(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(),
'"}'
);
}
// 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 == 137) return "pol";
if (block.chainid == 8453) return "base";
if (block.chainid == 42_161) return "arb1";
if (block.chainid == 204) return "opbnb"; // cspell:ignore 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 👍 / 👎.

}
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.

🚩 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.

}

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