Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add overloaded checkNSignatures #589

Merged
merged 4 commits into from
Aug 1, 2023
Merged
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
26 changes: 16 additions & 10 deletions contracts/Safe.sol
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,7 @@ contract Safe is
bytes32 txHash;
// Use scope here to limit variable lifetime and prevent `stack too deep` errors
{
bytes memory txHashData = encodeTransactionData(
// Transaction info
txHash = getTransactionHash( // Transaction info
to,
value,
data,
Expand All @@ -164,12 +163,10 @@ contract Safe is
gasToken,
refundReceiver,
// Signature info
nonce
// We use the post-increment here, so the current nonce value is used and incremented afterwards.
nonce++
akshay-ap marked this conversation as resolved.
Show resolved Hide resolved
);
// Increase nonce and execute transaction.
nonce++;
txHash = keccak256(txHashData);
checkSignatures(txHash, txHashData, signatures);
checkSignatures(txHash, "", signatures);
}
address guard = getGuard();
{
Expand Down Expand Up @@ -261,7 +258,7 @@ contract Safe is
uint256 _threshold = threshold;
// Check that a threshold is set
require(_threshold > 0, "GS001");
checkNSignatures(dataHash, data, signatures, _threshold);
checkNSignatures(msg.sender, dataHash, data, signatures, _threshold);
}

/**
Expand All @@ -270,12 +267,21 @@ contract Safe is
* The data parameter (bytes) is not used since v1.5.0 as it is not required anymore. Prior to v1.5.0,
* data parameter was used in contract signature validation flow using legacy EIP-1271.
* Version v1.5.0, uses dataHash parameter instead of data with updated EIP-1271 implementation.
* @param executor Address that executing the transaction.
* ⚠️⚠️⚠️ Make sure that the executor address is a legitmate executor.
* Incorrectly passed the executor might reduce the threshold by 1 signature. ⚠️⚠️⚠️
* @param dataHash Hash of the data (could be either a message hash or transaction hash)
* @param signatures Signature data that should be verified.
* Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
* @param requiredSignatures Amount of required valid signatures.
*/
function checkNSignatures(bytes32 dataHash, bytes memory /* data */, bytes memory signatures, uint256 requiredSignatures) public view {
function checkNSignatures(
address executor,
bytes32 dataHash,
bytes memory /* data */,
bytes memory signatures,
uint256 requiredSignatures
) public view {
// Check that the provided signature data is not too short
require(signatures.length >= requiredSignatures.mul(65), "GS020");
// There cannot be an owner with address 0.
Expand Down Expand Up @@ -323,7 +329,7 @@ contract Safe is
// When handling approved hashes the address of the approver is encoded into r
currentOwner = address(uint160(uint256(r)));
// Hashes are automatically approved by the sender of the message or when they have been pre-approved via a separate transaction
require(msg.sender == currentOwner || approvedHashes[currentOwner][dataHash] != 0, "GS025");
require(executor == currentOwner || approvedHashes[currentOwner][dataHash] != 0, "GS025");
} else if (v > 30) {
// If v > 30 then default va (27,28) has been adjusted for eth_sign flow
// To support eth_sign and similar we adjust v and hash the messageHash with the Ethereum message prefix before applying ecrecover
Expand Down
18 changes: 17 additions & 1 deletion contracts/handler/CompatibilityFallbackHandler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@ pragma solidity >=0.7.0 <0.9.0;
import "./TokenCallbackHandler.sol";
import "../interfaces/ISignatureValidator.sol";
import "../Safe.sol";
import "./HandlerContext.sol";

/**
* @title Compatibility Fallback Handler - Provides compatibility between pre 1.3.0 and 1.3.0+ Safe contracts.
* @author Richard Meissner - @rmeissner
*/
contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidator {
contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidator, HandlerContext {
// keccak256("SafeMessage(bytes message)");
bytes32 private constant SAFE_MSG_TYPEHASH = 0x60b3cbf8b4a223d68d641b3b6ddf9a298e7f33710cf3d3a9d1146b5a6150fbca;

Expand Down Expand Up @@ -154,4 +155,19 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
}
}
}

/**
* @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise.
* @dev Since the EIP-1271 does an external call, be mindful of reentrancy attacks.
* The function was moved to the fallback handler as a part of
* 1.5.0 contract upgrade. It used to be a part of the Safe core contract, but
* was replaced by the same function that accepts the executor as a parameter.
* @param dataHash Hash of the data (could be either a message hash or transaction hash)
* @param signatures Signature data that should be verified.
* Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
* @param requiredSignatures Amount of required valid signatures.
*/
function checkNSignatures(bytes32 dataHash, bytes memory, bytes memory signatures, uint256 requiredSignatures) public view {
Safe(payable(_manager())).checkNSignatures(_msgSender(), dataHash, "", signatures, requiredSignatures);
}
}
Loading
Loading