Verify all provided signatures when executing a transaction

Safe Development Contracts
1

(Not sure this belongs here, feel free to propose a better category)

ABSTRACT

Currently the execTransaction function only ends up verifying threshold signatures even if more are provided.
I can’t see any reason why a user would provide additionnal signatures on top of the defined Safe’s threshold if he does not want them also verified.

My proposal is to always verify all provided signatures.

This would allow for instance the implementation of Guards enforcing to easily require additionnal signatures for specific transactions.

DETAILS

My reference here is commit c4feb7d on GitHub - safe-global/safe-smart-account: Safe allows secure management of blockchain assets.

This could for instance be done by modifying the checkSignatures function at line 276, instead of

// Load threshold to avoid multiple storage loads
uint256 _threshold = threshold;
// Check that a threshold is set
if (_threshold == 0) revertWithError("GS001");
checkNSignatures(executor, dataHash, signatures, _threshold);

Use this

uint256 nbSignatures = signatures / 65;
// Check that a threshold is set
if (threshold== 0) revertWithError("GS001");
// Check that the provided signature data is not too short
if (nbSignatures<threshold) revertWithError("GS020");
checkNSignatures(executor, dataHash, signatures, nbSignatures);
2 Likes
2

As good a place as any :smiley:. Also, thanks very much for posting :raising_hands:, its nice to have these kinds of technical discussions in the forum.

One use case is to pass additional context to guards. For example, a “cosigner guard” that verifies an additional cosigner (separate from the Safe owners and threshold) approved a transaction can be implemented where it expects the additional signature to be appended to the end of the signature bytes after the owners’ signatures. We have an existing contract that makes use of this:

This is functionally different from just adding the cosigner as an owner and increasing the threshold, with the former the cosigner is mandatory for all transactions, with the latter, the cosigner is an optional additional signer for a transaction and can be bypassed if a threshold+1 of owners sign the transaction (unless you pair it with a guard that checks each of the signatures again for a specific owner, which is much less efficient).

So, appending additional data to signatures is the current recommended way to attach additional context for Safe transaction guards. IMO, this is a very compelling use case and is worth leaving in.

This unfortunately won’t quite work, as it does not account for smart contract Safe owners (over EIP-1271), since the signature size is dynamic and they have special encoding scheme that expects data past signatures[threshold * 65:].

1 Like
3

I see you point.

This is something I had in mind but I saw it more like a hack. There is nothing in the current Safe documentation that says the main contract will not at some point either verify all signatures, alter the signatures argument above the threshold… It feels like using an undocumented internal API, would be nice to make it somewhat “official”

Can you provide more information on that? Where can I find an example to parse such signatures?
So the current Safe implementation does not support these dynamic size signatures either I guess?

Line 294 of Safe.sol:

// Check that the provided signature data is not too short
if (signatures.length < requiredSignatures.mul(65)) revertWithError("GS020");
4

This is good feedback, it definitely makes sense to add this to the docs!

It is covered in the docs: Signatures – Safe Docs

If you are more of a “code is documentation” person, this function describes the encoding: safe-smart-account/src/utils/execution.ts at 6c4b4fd8f160186fbc6a88a9d864578eea815bdd · safe-global/safe-smart-account · GitHub

It does: safe-smart-account/contracts/Safe.sol at 6c4b4fd8f160186fbc6a88a9d864578eea815bdd · safe-global/safe-smart-account · GitHub

5

Cool, I had missed this part.

6

Let me describe my original use case: a guard which for some clearly defined actions requires a higher number of signatures than the Safe’s threshold. Currently, the only way to achieve this is for the guard to call (please correct me if I am wrong!)

checkNSignatures(safeTxHash, data, signatures, customThreshold);

But that wastes quite some gas: the first threshold signatures are verified both by the Safe and the Guard :frowning: … You pointed out that having the Safe verify all provided signature is first not that simple and then just not a good idea as it will break some Guard implementations.

Another solution would be to allow the Guard to “continue” the Safe’s signature verification.

The only information the guard needs to do that is the last owner verified by the Safe within checkNSignatures.
Then ideally the Guard calls
checkNSignatures(safeTxHash, data, signatures, customThreshold, lastOwner);

// We modify the current checkNSignatures implementation line 287
function checkNSignatures(
        address executor,
        bytes32 dataHash,
        bytes memory signatures,
        uint256 requiredSignatures,
        address lastOwner
    ) public view override {
...
  // address lastOwner = address(0);  Remove this line
...
  uint256 start = requiredSignatures > threshold ? threshold : 0;  // Add this line
  for (i = start; i < requiredSignatures; ++i) {  // Modify start value
...
  return lastOwner; // Will be provided to the Guard
}
// And we add this overloaded version of checkNSignatures to keep it all backward compatible
function checkNSignatures(
        address executor,
        bytes32 dataHash,
        bytes memory signatures,
        uint256 requiredSignatures
    ) public view override {
  checkNSignatures(safeTxHash, data, signatures, requiredSignatures, address(0));
}

But that forces us to change checkTransaction so it takes the new last parameter lastOwner, maybe not such a great idea for only this use case.

An alternative that does not require changing checkTransaction is to just resume verification at start = threshold - 1 and in this case we dont need the last owner, but we verify 1 signature twice - still much better that verifying threshold signatures twice.