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.

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

Check Signatures moved to Safe.sol #721

Merged
merged 4 commits into from
Jan 4, 2024
Merged

Check Signatures moved to Safe.sol #721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
71ff245
Transferring checkSignatures(...) to Safe
remedcu Dec 26, 2023
20a4bab
Added fmt script to package.json
remedcu Dec 26, 2023
2354477
Removed checkSignatures from CompatibilityFallbackHandler
remedcu Dec 26, 2023
2f2c56c
Tests edited after checkSignatures removal
remedcu Dec 26, 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
11 changes: 11 additions & 0 deletions contracts/Safe.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,17 @@ contract Safe is
* Can be packed ECDSA signature ({bytes32 r}{bytes32 s}{uint8 v}), contract signature (EIP-1271) or approved hash.
*/
function checkSignatures(bytes32 dataHash, bytes memory signatures) public view {
checkSignatures(dataHash, "", signatures);
}

/**
* @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise.
* @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.
* @dev This function makes it compatible with previous versions.
*/
function checkSignatures(bytes32 dataHash, bytes memory /* IGNORED */, bytes memory signatures) public view {
// Load threshold to avoid multiple storage loads
uint256 _threshold = threshold;
// Check that a threshold is set
Expand Down
17 changes: 0 additions & 17 deletions contracts/handler/CompatibilityFallbackHandler.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,21 +176,4 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
) public view {
Safe(payable(_manager())).checkNSignatures(_msgSender(), dataHash, 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 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 removes the raw encoded bytes data 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.
*/
function checkSignatures(bytes32 dataHash, bytes memory /* IGNORED */, bytes memory signatures) public view {
Safe safe = Safe(payable(_manager()));

uint256 threshold = safe.getThreshold();
safe.checkNSignatures(_msgSender(), dataHash, signatures, threshold);
}
}
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"lint:sol:prettier": "prettier 'contracts/**/*.sol' --list-different",
"lint:ts": "eslint 'src/**/*.ts' 'test/**/*.ts' --fix",
"lint:ts:prettier": "prettier 'src/**/*.ts' 'test/**/*.ts' --list-different",
"fmt": "npm run fmt:sol && npm run fmt:ts",
"fmt:sol": "prettier 'contracts/**/*.sol' -w",
"fmt:ts": "prettier 'src/**/*.ts' 'test/**/*.ts' -w",
"prepack": "npm run build",
Expand Down
29 changes: 10 additions & 19 deletions test/core/Safe.Signatures.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,6 @@ describe("Safe", () => {
it("should fail if signature points into static part", async () => {
const {
safe,
safeWithCompatFbHandlerIface,
signers: [user1],
} = await setupTests();
const safeAddress = await safe.getAddress();
Expand All @@ -471,13 +470,12 @@ describe("Safe", () => {
"0000000000000000000000000000000000000000000000000000000000000020" +
"00" + // r, s, v
"0000000000000000000000000000000000000000000000000000000000000000"; // Some data to read
await expect(safeWithCompatFbHandlerIface.checkSignatures(txHash, "0x", signatures)).to.be.revertedWith("GS021");
await expect(safe["checkSignatures(bytes32,bytes,bytes)"](txHash, "0x", signatures)).to.be.revertedWith("GS021");
});

it("should fail if signatures data is not present", async () => {
const {
safe,
safeWithCompatFbHandlerIface,
signers: [user1],
} = await setupTests();
const safeAddress = await safe.getAddress();
Expand All @@ -492,14 +490,13 @@ describe("Safe", () => {
"0000000000000000000000000000000000000000000000000000000000000041" +
"00"; // r, s, v

await expect(safeWithCompatFbHandlerIface.checkSignatures(txHash, txHashData, signatures)).to.be.revertedWith("GS022");
await expect(safe["checkSignatures(bytes32,bytes,bytes)"](txHash, txHashData, signatures)).to.be.revertedWith("GS022");
});

it("should fail if signatures data is too short", async () => {
const {
safe,
signers: [user1],
safeWithCompatFbHandlerIface,
} = await setupTests();
const safeAddress = await safe.getAddress();
const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
Expand All @@ -514,37 +511,34 @@ describe("Safe", () => {
"00" + // r, s, v
"0000000000000000000000000000000000000000000000000000000000000020"; // length

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

it("should not be able to use different chainId for signing", async () => {
const {
signers: [user1],
compatFallbackHandler,
} = await setupTests();
const safe = await getSafeWithOwners([user1.address], 1, await compatFallbackHandler.getAddress());
const safe = await getSafeWithOwners([user1.address]);
const safeAddress = await safe.getAddress();
const safeWithCompatFbHandlerIface = await getCompatFallbackHandler(safeAddress);
const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
const txHashData = preimageSafeTransactionHash(safeAddress, tx, await chainId());
const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId());
const signatures = buildSignatureBytes([await safeSignTypedData(user1, safeAddress, tx, 1)]);
await expect(safeWithCompatFbHandlerIface.checkSignatures(txHash, txHashData, signatures)).to.be.revertedWith("GS026");
await expect(safe["checkSignatures(bytes32,bytes,bytes)"](txHash, txHashData, signatures)).to.be.revertedWith("GS026");
});

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

it("should revert if not the required amount of signature data is provided", async () => {
Expand All @@ -558,11 +552,10 @@ describe("Safe", () => {
await compatFallbackHandler.getAddress(),
);
const safeAddress = await safe.getAddress();
const safeWithCompatFbHandlerIface = await getCompatFallbackHandler(safeAddress);
const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
const txHashData = preimageSafeTransactionHash(safeAddress, tx, await chainId());
const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId());
await expect(safeWithCompatFbHandlerIface.checkSignatures(txHash, txHashData, "0x")).to.be.revertedWith("GS020");
await expect(safe["checkSignatures(bytes32,bytes,bytes)"](txHash, txHashData, "0x")).to.be.revertedWith("GS020");
});

it("should not be able to use different signature type of same owner", async () => {
Expand All @@ -576,7 +569,6 @@ describe("Safe", () => {
await compatFallbackHandler.getAddress(),
);
const safeAddress = await safe.getAddress();
const safeWithCompatFbHandlerIface = await getCompatFallbackHandler(safeAddress);
const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
const txHashData = preimageSafeTransactionHash(safeAddress, tx, await chainId());
const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId());
Expand All @@ -585,7 +577,7 @@ describe("Safe", () => {
await safeSignTypedData(user1, safeAddress, tx),
await safeSignTypedData(user3, safeAddress, tx),
]);
await expect(safeWithCompatFbHandlerIface.checkSignatures(txHash, txHashData, signatures)).to.be.revertedWith("GS026");
await expect(safe["checkSignatures(bytes32,bytes,bytes)"](txHash, txHashData, signatures)).to.be.revertedWith("GS026");
});

it("should be able to mix all signature types", async () => {
Expand All @@ -604,7 +596,6 @@ describe("Safe", () => {
const safeAddress = await safe.getAddress();
const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId());
const safeWithCompatFbHandlerIface = await getCompatFallbackHandler(safeAddress);

const safeMessageHash = calculateSafeMessageHash(signerSafeAddress, txHash, await chainId());
const signerSafeOwnerSignature = await signHash(user5, safeMessageHash);
Expand All @@ -618,7 +609,7 @@ describe("Safe", () => {
signerSafeSig,
]);

await safeWithCompatFbHandlerIface.checkSignatures(txHash, "0x", signatures);
await safe["checkSignatures(bytes32,bytes,bytes)"](txHash, "0x", signatures);
});
});

Expand Down
Loading