Skip to content

Commit 90d7caa

Browse files
authored
Update Compatibility Signature Verification Functions (safe-global#750)
I was trying to move the signature verification logic to use `calldata` instead of `memory` for the signature bytes and decided to make some small changes to the documentation around the legacy `checkSignatures` functions. Additionally, since we have additional code size, we were able to move the legacy `checkSignatures` back into the `Safe` contract so continued compatibility with existing Safes does not require specific fallback handlers.
1 parent 2474f63 commit 90d7caa

File tree

4 files changed

+88
-88
lines changed

4 files changed

+88
-88
lines changed

contracts/Safe.sol

+31-5
Original file line numberDiff line numberDiff line change
@@ -249,11 +249,6 @@ contract Safe is
249249

250250
// @inheritdoc ISafe
251251
function checkSignatures(bytes32 dataHash, bytes memory signatures) public view override {
252-
checkSignatures(dataHash, "", signatures);
253-
}
254-
255-
// @inheritdoc ISafe
256-
function checkSignatures(bytes32 dataHash, bytes memory /* IGNORED */, bytes memory signatures) public view override {
257252
// Load threshold to avoid multiple storage loads
258253
uint256 _threshold = threshold;
259254
// Check that a threshold is set
@@ -315,6 +310,37 @@ contract Safe is
315310
}
316311
}
317312

313+
/**
314+
* @notice Checks whether the signature provided is valid for the provided hash. Reverts otherwise.
315+
* The `data` parameter is completely ignored during signature verification.
316+
* @dev This function is provided for compatibility with previous versions.
317+
* Use `checkSignatures(bytes32,bytes)` instead.
318+
* @param dataHash Hash of the data (could be either a message hash or transaction hash).
319+
* @param data **IGNORED** The data pre-image.
320+
* @param signatures Signature data that should be verified.
321+
* Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
322+
*/
323+
function checkSignatures(bytes32 dataHash, bytes calldata data, bytes memory signatures) external view {
324+
data;
325+
checkSignatures(dataHash, signatures);
326+
}
327+
328+
/**
329+
* @notice Checks whether the signature provided is valid for the provided hash. Reverts otherwise.
330+
* The `data` parameter is completely ignored during signature verification.
331+
* @dev This function is provided for compatibility with previous versions.
332+
* Use `checkNSignatures(address,bytes32,bytes,uint256)` instead.
333+
* @param dataHash Hash of the data (could be either a message hash or transaction hash)
334+
* @param data **IGNORED** The data pre-image.
335+
* @param signatures Signature data that should be verified.
336+
* Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
337+
* @param requiredSignatures Amount of required valid signatures.
338+
*/
339+
function checkNSignatures(bytes32 dataHash, bytes calldata data, bytes memory signatures, uint256 requiredSignatures) external view {
340+
data;
341+
checkNSignatures(msg.sender, dataHash, signatures, requiredSignatures);
342+
}
343+
318344
// @inheritdoc ISafe
319345
function approveHash(bytes32 hashToApprove) external override {
320346
if (owners[msg.sender] == address(0)) revertWithError("GS030");

contracts/handler/CompatibilityFallbackHandler.sol

-20
Original file line numberDiff line numberDiff line change
@@ -157,24 +157,4 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
157157
}
158158
/* solhint-enable no-inline-assembly */
159159
}
160-
161-
/**
162-
* @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise.
163-
* @dev Since the EIP-1271 does an external call, be mindful of reentrancy attacks.
164-
* The function was moved to the fallback handler as a part of
165-
* 1.5.0 contract upgrade. It used to be a part of the Safe core contract, but
166-
* was replaced by the same function that also accepts an executor address.
167-
* @param dataHash Hash of the data (could be either a message hash or transaction hash)
168-
* @param signatures Signature data that should be verified.
169-
* Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
170-
* @param requiredSignatures Amount of required valid signatures.
171-
*/
172-
function checkNSignatures(
173-
bytes32 dataHash,
174-
bytes memory /* IGNORED */,
175-
bytes memory signatures,
176-
uint256 requiredSignatures
177-
) public view {
178-
ISafe(payable(_manager())).checkNSignatures(_msgSender(), dataHash, signatures, requiredSignatures);
179-
}
180160
}

contracts/interfaces/ISafe.sol

-9
Original file line numberDiff line numberDiff line change
@@ -82,15 +82,6 @@ interface ISafe is IModuleManager, IOwnerManager, IFallbackManager {
8282
*/
8383
function checkSignatures(bytes32 dataHash, bytes memory signatures) external view;
8484

85-
/**
86-
* @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise.
87-
* @param dataHash Hash of the data (could be either a message hash or transaction hash)
88-
* @param signatures Signature data that should be verified.
89-
* Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
90-
* @dev This function makes it compatible with previous versions.
91-
*/
92-
function checkSignatures(bytes32 dataHash, bytes memory /* IGNORED */, bytes memory signatures) external view;
93-
9485
/**
9586
* @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise.
9687
* @dev Since the EIP-1271 does an external call, be mindful of reentrancy attacks.

0 commit comments

Comments
 (0)