Skip to content

Commit

Permalink
Disable Pre-approved IsValidSignature
Browse files Browse the repository at this point in the history 
This commit disabes support for pre-approved signatures from
`msg.sender` for `isValidSignature` calls.

Note that we remove the new 1.5.0-only `checkSignatures` interface in
favour of one with `msg.sender` explicitely provided to avoid issues for
contracts that make use of this function.
  • Loading branch information
@nlordell
nlordell committed Dec 11, 2024
1 parent febab5e commit 9f29682
Show file tree
Hide file tree
Showing 7 changed files with 101 additions and 41 deletions.
8 changes: 4 additions & 4 deletions contracts/Safe.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ contract Safe is
// We use the post-increment here, so the current nonce value is used and incremented afterwards.
nonce++
);
checkSignatures(txHash, signatures);
checkSignatures(msg.sender, txHash, signatures);
}
address guard = getGuard();
{
Expand Down Expand Up @@ -267,12 +267,12 @@ contract Safe is
/**
* @inheritdoc ISafe
*/
function checkSignatures(bytes32 dataHash, bytes memory signatures) public view override {
function checkSignatures(address executor, bytes32 dataHash, bytes memory signatures) public view override {
// Load threshold to avoid multiple storage loads
uint256 _threshold = threshold;
// Check that a threshold is set
if (_threshold == 0) revertWithError("GS001");
checkNSignatures(msg.sender, dataHash, signatures, _threshold);
checkNSignatures(executor, dataHash, signatures, _threshold);
}

/**
Expand Down Expand Up @@ -343,7 +343,7 @@ contract Safe is
*/
function checkSignatures(bytes32 dataHash, bytes calldata data, bytes memory signatures) external view {
data;
checkSignatures(dataHash, signatures);
checkSignatures(msg.sender, dataHash, signatures);
}

/**
Expand Down
4 changes: 3 additions & 1 deletion contracts/handler/CompatibilityFallbackHandler.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
if (_signature.length == 0) {
require(safe.signedMessages(messageHash) != 0, "Hash not approved");
} else {
safe.checkSignatures(messageHash, _signature);
// We explicitly do not allow caller approved sigantures for EIP-1271. This is done by setting the executor
// address to `0` which can never be an owner of the Safe.
safe.checkSignatures(address(0), messageHash, _signature);
}
return EIP1271_MAGIC_VALUE;
}
Expand Down
2 changes: 1 addition & 1 deletion contracts/handler/extensible/SignatureVerifierMuxer.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ abstract contract SignatureVerifierMuxer is ExtensibleBase, ERC1271, ISignatureV
require(safe.signedMessages(messageHash) != 0, "Hash not approved");
} else {
// threshold signatures
safe.checkSignatures(messageHash, messageData, signature);
safe.checkSignatures(address(0), messageHash, signature);
}
magic = ERC1271.isValidSignature.selector;
}
Expand Down
7 changes: 5 additions & 2 deletions contracts/interfaces/ISafe.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,15 @@ interface ISafe is IModuleManager, IGuardManager, IOwnerManager, IFallbackManage
) external payable returns (bool success);

/**
* @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise.
* @notice Checks whether the signature provided is valid for the provided data and hash and executor. Reverts otherwise.
* @param executor Address that executes the transaction.
* ⚠️⚠️⚠️ Make sure that the executor address is a legitimate 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.
*/
function checkSignatures(bytes32 dataHash, bytes memory signatures) external view;
function checkSignatures(address executor, bytes32 dataHash, bytes memory signatures) external view;

/**
* @notice Checks whether the signature provided is valid for the provided data and hash. Reverts otherwise.
Expand Down
66 changes: 36 additions & 30 deletions test/core/Safe.Signatures.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,9 @@ describe("Safe", () => {
"0000000000000000000000000000000000000000000000000000000000000020" +
"00" + // r, s, v
"0000000000000000000000000000000000000000000000000000000000000000"; // Some data to read
await expect(safe["checkSignatures(bytes32,bytes)"](txHash, signatures)).to.be.revertedWith("GS021");
await expect(safe["checkSignatures(address,bytes32,bytes)"](hre.ethers.ZeroAddress, txHash, signatures)).to.be.revertedWith(
"GS021",
);
});

it("should fail if signatures data is not present", async () => {
Expand All @@ -352,7 +354,9 @@ describe("Safe", () => {
"0000000000000000000000000000000000000000000000000000000000000041" +
"00"; // r, s, v

await expect(safe["checkSignatures(bytes32,bytes)"](txHash, signatures)).to.be.revertedWith("GS022");
await expect(safe["checkSignatures(address,bytes32,bytes)"](hre.ethers.ZeroAddress, txHash, signatures)).to.be.revertedWith(
"GS022",
);
});

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

await expect(safe["checkSignatures(bytes32,bytes)"](txHash, signatures)).to.be.revertedWith("GS023");
await expect(safe["checkSignatures(address,bytes32,bytes)"](hre.ethers.ZeroAddress, txHash, signatures)).to.be.revertedWith(
"GS023",
);
});

it("should not be able to use different chainId for signing", async () => {
Expand All @@ -384,7 +390,9 @@ describe("Safe", () => {
const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId());
const signatures = buildSignatureBytes([await safeSignTypedData(user1, safeAddress, tx, 1)]);
await expect(safe["checkSignatures(bytes32,bytes)"](txHash, signatures)).to.be.revertedWith("GS026");
await expect(safe["checkSignatures(address,bytes32,bytes)"](hre.ethers.ZeroAddress, txHash, signatures)).to.be.revertedWith(
"GS026",
);
});

it("if not msg.sender on-chain approval is required", async () => {
Expand All @@ -397,7 +405,9 @@ describe("Safe", () => {
const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId());
const signatures = buildSignatureBytes([await safeApproveHash(user1, safe, tx, true)]);
await expect(user2Safe["checkSignatures(bytes32,bytes)"](txHash, signatures)).to.be.revertedWith("GS025");
await expect(
user2Safe["checkSignatures(address,bytes32,bytes)"](hre.ethers.ZeroAddress, txHash, signatures),
).to.be.revertedWith("GS025");
});

it("should revert if threshold is not set", async () => {
Expand All @@ -406,7 +416,7 @@ describe("Safe", () => {
const safeAddress = await safe.getAddress();
const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId());
await expect(safe["checkSignatures(bytes32,bytes)"](txHash, "0x")).to.be.revertedWith("GS001");
await expect(safe["checkSignatures(address,bytes32,bytes)"](hre.ethers.ZeroAddress, txHash, "0x")).to.be.revertedWith("GS001");
});

it("should revert if not the required amount of signature data is provided", async () => {
Expand All @@ -417,7 +427,7 @@ describe("Safe", () => {
const safeAddress = await safe.getAddress();
const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId());
await expect(safe["checkSignatures(bytes32,bytes)"](txHash, "0x")).to.be.revertedWith("GS020");
await expect(safe["checkSignatures(address,bytes32,bytes)"](hre.ethers.ZeroAddress, txHash, "0x")).to.be.revertedWith("GS020");
});

it("should not be able to use different signature type of same owner", async () => {
Expand All @@ -433,7 +443,9 @@ describe("Safe", () => {
await safeSignTypedData(user1, safeAddress, tx),
await safeSignTypedData(user3, safeAddress, tx),
]);
await expect(safe["checkSignatures(bytes32,bytes)"](txHash, signatures)).to.be.revertedWith("GS026");
await expect(safe["checkSignatures(address,bytes32,bytes)"](hre.ethers.ZeroAddress, txHash, signatures)).to.be.revertedWith(
"GS026",
);
});

it("should be able to mix all signature types", async () => {
Expand All @@ -448,11 +460,9 @@ describe("Safe", () => {
fallbackHandler: compatFallbackHandlerAddress,
});
const signerSafeAddress = await signerSafe.getAddress();
const safe = (
await getSafe({
owners: [user1.address, user2.address, user3.address, user4.address, signerSafeAddress],
})
).connect(user1);
const safe = await getSafe({
owners: [user1.address, user2.address, user3.address, user4.address, signerSafeAddress],
});
const safeAddress = await safe.getAddress();
const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId());
Expand All @@ -469,7 +479,7 @@ describe("Safe", () => {
signerSafeSig,
]);

await safe["checkSignatures(bytes32,bytes)"](txHash, signatures);
await safe["checkSignatures(address,bytes32,bytes)"](user1.address, txHash, signatures);
});
});

Expand Down Expand Up @@ -612,13 +622,11 @@ describe("Safe", () => {
fallbackHandler: compatFallbackHandlerAddress,
});
const signerSafeAddress = await signerSafe.getAddress();
const safe = (
await getSafe({
owners: [user1.address, user2.address, user3.address, user4.address, signerSafeAddress],
threshold: 5,
fallbackHandler: compatFallbackHandlerAddress,
})
).connect(user1);
const safe = await getSafe({
owners: [user1.address, user2.address, user3.address, user4.address, signerSafeAddress],
threshold: 5,
fallbackHandler: compatFallbackHandlerAddress,
});
const safeAddress = await safe.getAddress();
const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId());
Expand All @@ -635,7 +643,7 @@ describe("Safe", () => {
signerSafeSig,
]);

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

Expand Down Expand Up @@ -1021,13 +1029,11 @@ describe("Safe", () => {
fallbackHandler: compatFallbackHandlerAddress,
});
const signerSafeAddress = await signerSafe.getAddress();
const safe = (
await getSafe({
owners: [user1.address, user2.address, user3.address, user4.address, signerSafeAddress],
threshold: 5,
fallbackHandler: compatFallbackHandlerAddress,
})
).connect(user1);
const safe = await getSafe({
owners: [user1.address, user2.address, user3.address, user4.address, signerSafeAddress],
threshold: 5,
fallbackHandler: compatFallbackHandlerAddress,
});
const safeAddress = await safe.getAddress();
const tx = buildSafeTransaction({ to: safeAddress, nonce: await safe.nonce() });
const txHash = calculateSafeTransactionHash(safeAddress, tx, await chainId());
Expand All @@ -1044,7 +1050,7 @@ describe("Safe", () => {
signerSafeSig,
]);

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

it("should be able to require no signatures", async () => {
Expand Down
34 changes: 31 additions & 3 deletions test/handlers/CompatibilityFallbackHandler.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,37 @@ describe("CompatibilityFallbackHandler", () => {

const signerSafeSig = buildContractSignature(signerSafeAddress, signerSafeOwnerSignature.data);

expect(
await validator.isValidSignature.staticCall(dataHash, buildSignatureBytes([typedDataSig, ethSignSig, signerSafeSig])),
).to.be.eq("0x1626ba7e");
for (const signatures of [
[typedDataSig, ethSignSig],
[typedDataSig, signerSafeSig],
[ethSignSig, signerSafeSig],
]) {
expect(await validator.isValidSignature.staticCall(dataHash, buildSignatureBytes(signatures))).to.be.eq("0x1626ba7e");
}
});

it("should not accept pre-approved signatures", async () => {
const {
validator,
signers: [user1, user2],
} = await setupTests();
const validatorAddress = await validator.getAddress();
const dataHash = ethers.keccak256("0xbaddad");
const user1Signature = {
signer: user1.address,
data: ethers.solidityPacked(["uint256", "uint256", "uint8"], [user1.address, 0, 1]),
};
const user2Signature = {
signer: user2.address,
data: await user2.signTypedData(
{ verifyingContract: validatorAddress, chainId: await chainId() },
EIP712_SAFE_MESSAGE_TYPE,
{ message: dataHash },
),
};

const signatures = buildSignatureBytes([user1Signature, user2Signature]);
await expect(validator.connect(user1).isValidSignature.staticCall(dataHash, signatures)).to.be.reverted;
});
});

Expand Down
21 changes: 21 additions & 0 deletions test/handlers/ExtensibleFallbackHandler.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,27 @@ describe("ExtensibleFallbackHandler", () => {
);
});

it("should not accept pre-approved signatures", async () => {
const { user1, user2, validator } = await setupTests();
const validatorAddress = await validator.getAddress();
const dataHash = ethers.keccak256("0xbaddad");
const user1Signature = {
signer: user1.address,
data: ethers.solidityPacked(["uint256", "uint256", "uint8"], [user1.address, 0, 1]),
};
const user2Signature = {
signer: user2.address,
data: await user2.signTypedData(
{ verifyingContract: validatorAddress, chainId: await chainId() },
EIP712_SAFE_MESSAGE_TYPE,
{ message: dataHash },
),
};

const signatures = buildSignatureBytes([user1Signature, user2Signature]);
await expect(validator.connect(user1).isValidSignature.staticCall(dataHash, signatures)).to.be.reverted;
});

it("should send EIP-712 context to custom verifier", async () => {
const { user1, user2, safe, validator, revertVerifier } = await setupTests();
const domainSeparator = ethers.keccak256("0xdeadbeef");
Expand Down

0 comments on commit 9f29682

Please sign in to comment.