Skip to content

SignMessagePolicy contract - #26

Open
remedcu wants to merge 2 commits into
mainfrom
signMessagePolicy
Open

SignMessagePolicy contract#26
remedcu wants to merge 2 commits into
mainfrom
signMessagePolicy

Conversation

@remedcu

@remedcu remedcu commented Oct 23, 2025

Copy link
Copy Markdown
Collaborator

TLDR

Created a signMessage policy based on a Twitter reply: https://x.com/yodl_meister/status/1980580069480493279

To Do:

  • Testing
  • Deployment
  • Safe App support

LLM Description

This pull request introduces a new policy contract, SignMessagePolicy, which enforces strict rules for signing messages in the system. The contract ensures that only messages with explicitly allowed domain and type hashes can be signed, and validates the message hash before permitting the operation. The implementation includes error handling and configuration logic for managing allowed domains and types.

New policy contract for signing messages:

  • Added SignMessagePolicy contract in contracts/policies/SignMessagePolicy.sol, enforcing that only signMessage operations with approved domain and type hashes are allowed.
  • Implemented a mapping to store allowed domain and primary type hashes per Safe, ensuring granular control over permitted signatures.
  • Added error handling for unauthorized domain/type hashes and invalid message hashes, improving security and debuggability.
  • Provided configuration functionality to set allowed domain/type hashes for each Safe, enabling flexible policy management.

@remedcu
remedcu requested a review from Copilot October 23, 2025 07:48
@remedcu remedcu self-assigned this Oct 23, 2025

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new SignMessagePolicy contract that enforces strict validation rules for signing messages through Safe wallets. The policy ensures only pre-approved EIP-712 structured data (identified by domain and primary type hashes) can be signed.

Key Changes:

  • Implements a whitelist-based policy requiring domain and type hash approval per Safe
  • Validates that message hashes match the expected EIP-712 structured data format
  • Provides configuration interface to set allowed domain/type hash combinations

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +68 to +69
require(
messageToSign.length == 32 && bytes32(abi.decode(messageToSign, (bytes32))) == messageHash,

Copilot AI Oct 23, 2025

Copy link

Choose a reason for hiding this comment

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

The double decoding of messageToSign is incorrect. First abi.decode(data[4:], (bytes)) decodes to bytes, then abi.decode(messageToSign, (bytes32)) tries to ABI-decode those bytes. If messageToSign is exactly 32 bytes representing a hash, it should be cast directly with bytes32(messageToSign) using inline assembly or by copying the bytes, not double-decoded.

Suggested change
require(
messageToSign.length == 32 && bytes32(abi.decode(messageToSign, (bytes32))) == messageHash,
bytes32 messageToSignHash;
assembly {
messageToSignHash := mload(add(messageToSign, 32))
}
require(
messageToSign.length == 32 && messageToSignHash == messageHash,

Copilot uses AI. Check for mistakes.
* @inheritdoc IPolicy
* @dev This policy requires configuration with allowed domain and type hashes.
*/
function configure(address safe, AccessSelector.T selector, bytes memory data) external override returns (bool) {

Copilot AI Oct 23, 2025

Copy link

Choose a reason for hiding this comment

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

The configure function lacks access control. Any caller can add allowed domain/type hashes for any Safe address. This should verify that msg.sender is authorized to configure the policy for the given Safe, or it should be restricted to the Safe itself or a designated administrator.

Suggested change
function configure(address safe, AccessSelector.T selector, bytes memory data) external override returns (bool) {
function configure(address safe, AccessSelector.T selector, bytes memory data) external override returns (bool) {
require(msg.sender == safe, "Not authorized");

Copilot uses AI. Check for mistakes.
Comment on lines +81 to +83
for (uint256 i = 0; i < domains.length; i++) {
_domains[safe][domains[i].domainHash][domains[i].primaryTypeHash] = true;
}

Copilot AI Oct 23, 2025

Copy link

Choose a reason for hiding this comment

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

The configuration logic only allows adding domain/type hash pairs but provides no mechanism to revoke or remove them. Once a domain/type combination is approved, it cannot be disabled. Consider adding a boolean field to SignatureDomain struct or a separate removal function to support policy updates.

Copilot uses AI. Check for mistakes.
*/
function configure(address safe, AccessSelector.T selector, bytes memory data) external override returns (bool) {
SignatureDomain[] memory domains = abi.decode(data, (SignatureDomain[]));
for (uint256 i = 0; i < domains.length; i++) {

Copilot AI Oct 23, 2025

Copy link

Choose a reason for hiding this comment

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

Unbounded loop over domains.length could exceed block gas limits if a large array is passed. Consider adding a reasonable upper limit check or implementing pagination for configuring large numbers of domain/type pairs.

Copilot uses AI. Check for mistakes.
@akshay-ap

Copy link
Copy Markdown
Collaborator

No tests?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants