Skip to content

Add overloaded checkNSignatures #589

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.

Sign up for GitHub

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

Merged
mmv08 merged 4 commits into from
Aug 1, 2023
Merged

Add overloaded checkNSignatures #589

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8088750
Add overloaded checkNSignatures
mmv08 Jul 13, 2023
6e9ad8a
Merge branch 'main' of https://github.com/safe-global/safe-contracts …
mmv08 Jul 17, 2023
d127220
add legacy checknsignatures tests
mmv08 Jul 18, 2023
df4f93d
Merge branch 'release/v1.5.0' of https://github.com/safe-global/safe-…
mmv08 Jul 18, 2023
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
Next Next commit
Add overloaded checkNSignatures
  • Loading branch information
@mmv08
mmv08 committed Jul 13, 2023
commit 808875098b0acfca3ffc3ddfe3ec695ab0e79d94
36 changes: 27 additions & 9 deletions contracts/Safe.sol
View file
Open in desktop
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,9 @@ contract Safe is
gasToken,
refundReceiver,
// Signature info
nonce
nonce++
);
// Increase nonce and execute transaction.
nonce++;
txHash = keccak256(txHashData);
checkSignatures(txHash, txHashData, signatures);
checkSignatures(txHash, "", signatures);
}
address guard = getGuard();
{
Expand Down Expand Up @@ -264,18 +260,40 @@ contract Safe is
checkNSignatures(dataHash, data, signatures, _threshold);
}

/**
* @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.
* @param dataHash Hash of the data (could be either a message hash or transaction hash)
* @param data That should be signed (this is passed to an external validator contract)
* @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 {
return checkNSignatures(msg.sender, dataHash, data, signatures, requiredSignatures);
}

/**
* @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 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 +341,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
96 changes: 73 additions & 23 deletions test/core/Safe.Signatures.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ describe("Safe", async () => {
it("should fail if signature points into static part", async () => {
const { safe } = await setupTests();
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId());

const txHash = calculateSafeTransactionHash(safe, tx, await chainId());
const signatures =
"0x" +
Expand Down Expand Up @@ -365,11 +365,11 @@ describe("Safe", async () => {
});
});

describe("checkSignatures", async () => {
describe("checkNSignatures", async () => {
it("should fail if signature points into static part", async () => {
const { safe } = await setupTests();

const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId());
const txHash = calculateSafeTransactionHash(safe, tx, await chainId());
const signatures =
"0x" +
Expand All @@ -378,13 +378,12 @@ describe("Safe", async () => {
"0000000000000000000000000000000000000000000000000000000000000020" +
"00" + // r, s, v
"0000000000000000000000000000000000000000000000000000000000000000"; // Some data to read
await expect(safe.checkNSignatures(txHash, txHashData, signatures, 1)).to.be.revertedWith("GS021");
await expect(safe["checkNSignatures(bytes32,bytes,bytes,uint256)"](txHash, "0x", signatures, 1)).to.be.revertedWith("GS021");
});

it("should fail if signatures data is not present", async () => {
const { safe } = await setupTests();
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId());
const txHash = calculateSafeTransactionHash(safe, tx, await chainId());

const signatures =
Expand All @@ -394,7 +393,7 @@ describe("Safe", async () => {
"0000000000000000000000000000000000000000000000000000000000000041" +
"00"; // r, s, v

await expect(safe.checkNSignatures(txHash, txHashData, signatures, 1)).to.be.revertedWith("GS022");
await expect(safe["checkNSignatures(bytes32,bytes,bytes,uint256)"](txHash, "0x", signatures, 1)).to.be.revertedWith("GS022");
});

it("should fail if signatures data is too short", async () => {
Expand All @@ -411,50 +410,51 @@ describe("Safe", async () => {
"00" + // r, s, v
"0000000000000000000000000000000000000000000000000000000000000020"; // length

await expect(safe.checkNSignatures(txHash, txHashData, signatures, 1)).to.be.revertedWith("GS023");
await expect(safe["checkNSignatures(bytes32,bytes,bytes,uint256)"](txHash, txHashData, signatures, 1)).to.be.revertedWith(
"GS023",
);
});

it("should not be able to use different chainId for signing", async () => {
await setupTests();
const safe = await getSafeWithOwners([user1.address]);
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId());

const txHash = calculateSafeTransactionHash(safe, tx, await chainId());
const signatures = buildSignatureBytes([await safeSignTypedData(user1, safe, tx, 1)]);
await expect(safe.checkNSignatures(txHash, txHashData, signatures, 1)).to.be.revertedWith("GS026");
await expect(safe["checkNSignatures(bytes32,bytes,bytes,uint256)"](txHash, "0x", signatures, 1)).to.be.revertedWith("GS026");
});

it("if not msg.sender on-chain approval is required", async () => {
const { safe } = await setupTests();
const user2Safe = safe.connect(user2);
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId());
const txHash = calculateSafeTransactionHash(safe, tx, await chainId());
const signatures = buildSignatureBytes([await safeApproveHash(user1, safe, tx, true)]);
await expect(user2Safe.checkNSignatures(txHash, txHashData, signatures, 1)).to.be.revertedWith("GS025");
await expect(user2Safe["checkNSignatures(bytes32,bytes,bytes,uint256)"](txHash, "0x", signatures, 1)).to.be.revertedWith(
"GS025",
);
});

it("should revert if not the required amount of signature data is provided", async () => {
await setupTests();
const safe = await getSafeWithOwners([user1.address, user2.address, user3.address]);
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId());
const txHash = calculateSafeTransactionHash(safe, tx, await chainId());
await expect(safe.checkNSignatures(txHash, txHashData, "0x", 1)).to.be.revertedWith("GS020");
await expect(safe["checkNSignatures(bytes32,bytes,bytes,uint256)"](txHash, "0x", "0x", 1)).to.be.revertedWith("GS020");
});

it("should not be able to use different signature type of same owner", async () => {
await setupTests();
const safe = await getSafeWithOwners([user1.address, user2.address, user3.address]);
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId());
const txHash = calculateSafeTransactionHash(safe, tx, await chainId());
const signatures = buildSignatureBytes([
await safeApproveHash(user1, safe, tx),
await safeSignTypedData(user1, safe, tx),
await safeSignTypedData(user3, safe, tx),
]);
await expect(safe.checkNSignatures(txHash, txHashData, signatures, 3)).to.be.revertedWith("GS026");
await expect(safe["checkNSignatures(bytes32,bytes,bytes,uint256)"](txHash, "0x", signatures, 3)).to.be.revertedWith("GS026");
});

it("should be able to mix all signature types", async () => {
Expand All @@ -477,45 +477,95 @@ describe("Safe", async () => {
signerSafeSig,
]);

await safe.checkNSignatures(txHash, "0x", signatures, 5);
await safe["checkNSignatures(bytes32,bytes,bytes,uint256)"](txHash, "0x", signatures, 5);
});

it("should be able to require no signatures", async () => {
await setupTests();
const safe = await getSafeTemplate();
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId());
const txHash = calculateSafeTransactionHash(safe, tx, await chainId());

await safe.checkNSignatures(txHash, txHashData, "0x", 0);
await safe["checkNSignatures(bytes32,bytes,bytes,uint256)"](txHash, "0x", "0x", 0);
});

it("should be able to require less signatures than the threshold", async () => {
await setupTests();
const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address]);
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId());
const txHash = calculateSafeTransactionHash(safe, tx, await chainId());
const signatures = buildSignatureBytes([await safeSignTypedData(user3, safe, tx)]);

await safe.checkNSignatures(txHash, txHashData, signatures, 1);
await safe["checkNSignatures(bytes32,bytes,bytes,uint256)"](txHash, "0x", signatures, 1);
});

it("should be able to require more signatures than the threshold", async () => {
await setupTests();
const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address], 2);
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId());
const txHash = calculateSafeTransactionHash(safe, tx, await chainId());
const signatures = buildSignatureBytes([
await safeApproveHash(user1, safe, tx, true),
await safeApproveHash(user4, safe, tx),
await safeSignTypedData(user2, safe, tx),
]);
// Should fail as only 3 signatures are provided
await expect(safe.checkNSignatures(txHash, txHashData, signatures, 4)).to.be.revertedWith("GS020");
await expect(safe["checkNSignatures(bytes32,bytes,bytes,uint256)"](txHash, "0x", signatures, 4)).to.be.revertedWith("GS020");

await safe["checkNSignatures(bytes32,bytes,bytes,uint256)"](txHash, "0x", signatures, 3);
});

it("should use msg.sender executing the check", async () => {
await setupTests();

const safe = await getSafeWithOwners([user1.address]);
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
const txHash = calculateSafeTransactionHash(safe, tx, await chainId());

const signatures = buildSignatureBytes([await safeApproveHash(user1, safe, tx, true)]);
const safeConnectUser2 = safe.connect(user2);

await expect(safeConnectUser2["checkNSignatures(bytes32,bytes,bytes,uint256)"](txHash, "0x", signatures, 1)).to.be.revertedWith(
"GS025",
);
});
});

describe("checkNSignatures (overloaded)", async () => {
it("Should accept an arbitrary msg.sender", async () => {
await setupTests();

const safe = await getSafeWithOwners([user1.address]);
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
const txHash = calculateSafeTransactionHash(safe, tx, await chainId());

const signatures = buildSignatureBytes([await safeApproveHash(user1, safe, tx, true)]);
const safeConnectUser2 = safe.connect(user2);

await safeConnectUser2["checkNSignatures(address,bytes32,bytes,bytes,uint256)"](user1.address, txHash, "0x", signatures, 1);
});

it("should behave exactly the same as the non-overloaded version", async () => {
await setupTests();
const compatFallbackHandler = await getCompatFallbackHandler();
const signerSafe = await getSafeWithOwners([user5.address], 1, compatFallbackHandler.address);
const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address, signerSafe.address]);
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() });
const txHash = calculateSafeTransactionHash(safe, tx, await chainId());

const safeMessageHash = calculateSafeMessageHash(signerSafe, txHash, await chainId());
const signerSafeOwnerSignature = await signHash(user5, safeMessageHash);
const signerSafeSig = buildContractSignature(signerSafe.address, signerSafeOwnerSignature.data);

const signatures = buildSignatureBytes([
await safeApproveHash(user1, safe, tx, true),
await safeApproveHash(user4, safe, tx),
await safeSignTypedData(user2, safe, tx),
await safeSignTypedData(user3, safe, tx),
signerSafeSig,
]);

await safe.checkNSignatures(txHash, txHashData, signatures, 3);
await safe["checkNSignatures(address,bytes32,bytes,bytes,uint256)"](user1.address, txHash, "0x", signatures, 5);
});
});
});
2 changes: 1 addition & 1 deletion yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9565,7 +9565,7 @@ underscore@1.9.1:
resolved "https://registry.yarnpkg.com/underscore/-/underscore-1.9.1.tgz#06dce34a0e68a7babc29b365b8e74b8925203961"
integrity sha512-5/4etnCkd9c8gwgowi5/om/mYO5ajCaOgdzj/oW+0eQV9WxKBDZw5+ycmKmeaTXjInS/W0BzpGLo2xR2aBwZdg==

undici@^5.14.0, undici@^5.4.0:
undici@^5.14.0:
version "5.21.2"
resolved "https://registry.yarnpkg.com/undici/-/undici-5.21.2.tgz#329f628aaea3f1539a28b9325dccc72097d29acd"
integrity sha512-f6pTQ9RF4DQtwoWSaC42P/NKlUjvezVvd9r155ohqkwFNRyBKM3f3pcty3ouusefNRyM25XhIQEbeQ46sZDJfQ==
Expand Down