Skip to content

Commit

Permalink
enforce dataHash to be hash of data
Browse files Browse the repository at this point in the history
  • Loading branch information
@mmv08
mmv08 committed Jan 24, 2023
1 parent 979d7f5 commit 981c8ff
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 35 deletions.
19 changes: 1 addition & 18 deletions contracts/Safe.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ import "./common/StorageAccessible.sol";
import "./interfaces/ISignatureValidator.sol";
import "./external/SafeMath.sol";

import "hardhat/console.sol";

/// @title Safe - A multisignature wallet with support for confirmations using signed messages based on ERC191.
/// @author Stefan George - <stefan@gnosis.io>
/// @author Richard Meissner - <richard@gnosis.io>
Expand Down Expand Up @@ -254,22 +252,14 @@ contract Safe is
bytes32 r;
bytes32 s;
uint256 i;
console.log("Starting signature check");
console.log("dataHash");
console.logBytes32(dataHash);
console.log("data");
console.logBytes(data);
console.log("signatures");
console.logBytes(signatures);
for (i = 0; i < requiredSignatures; i++) {
(v, r, s) = signatureSplit(signatures, i);
if (v == 0) {
require(keccak256(data) == dataHash, "GS027");
// If v is 0 then it is a contract signature
// When handling contract signatures the address of the contract is encoded into r
currentOwner = address(uint160(uint256(r)));
console.log("currentOwner");
console.log(currentOwner);

// Check that signature data pointer (s) is not pointing inside the static part of the signatures bytes
// This check is not completely accurate, since it is possible that more signatures than the threshold are send.
// Here we only check that the pointer is not pointing inside the part that is being processed
Expand All @@ -293,12 +283,7 @@ contract Safe is
// The signature data for contract signatures is appended to the concatenated signatures and the offset is stored in s
contractSignature := add(add(signatures, s), 0x20)
}
console.log("contractSignature");
console.logBytes(contractSignature);

console.log("checking eip-1271 signature");
require(ISignatureValidator(currentOwner).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "GS024");
console.log("checked eip-1271 signature");
} else if (v == 1) {
// If v is 1 then it is an approved hash
// When handling approved hashes the address of the approver is encoded into r
Expand All @@ -314,8 +299,6 @@ contract Safe is
// Use ecrecover with the messageHash for EOA signatures
currentOwner = ecrecover(dataHash, v, r, s);
}
console.log("Owner from the sig");
console.log(currentOwner);
require(currentOwner > lastOwner && owners[currentOwner] != address(0) && currentOwner != SENTINEL_OWNERS, "GS026");
lastOwner = currentOwner;
}
Expand Down
34 changes: 17 additions & 17 deletions test/core/Safe.Signatures.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,16 +430,19 @@ describe("Safe", async () => {
).to.be.revertedWith("GS026")
})

it.only('should be able to mix all signature types', async () => {
it('should be able to mix all signature types', 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])
console.log(await safe.isOwner(signerSafe.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 safeMessageHash = calculateSafeMessageHash(signerSafe, txHash, await chainId())

// IMPORTANT: because the safe uses the old EIP-1271 interface which uses `bytes` instead of `bytes32` for the message
// we need to use the pre-image of the transaction hash to calculate the message hash
const safeMessageHash = calculateSafeMessageHash(signerSafe, txHashData, await chainId())

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

Expand All @@ -450,11 +453,8 @@ describe("Safe", async () => {
await safeSignTypedData(user3, safe, tx),
signerSafeSig,
])

console.log({safeMessageHash, signerSafe: signerSafe.address, signature: signerSafeSig.data, signerSafeOwner: user5.address })
console.log({signatures})

await safe.checkNSignatures(txHash, txHashData, signatures, 4)
await safe.checkNSignatures(txHash, txHashData, signatures, 5)
})

it('should be able to require no signatures', async () => {
Expand Down Expand Up @@ -491,27 +491,27 @@ describe("Safe", async () => {
await safeApproveHash(user4, safe, tx),
await safeSignTypedData(user2, safe, tx)
])
// Should fail as only 3 signaures are provided
// Should fail as only 3 signatures are provided
await expect(
safe.checkNSignatures(txHash, txHashData, signatures, 4)
).to.be.revertedWith("GS020")

await safe.checkNSignatures(txHash, txHashData, signatures, 3)
})

it('should revert if the hash of the pre-image data and dataHash do not match', async () =>{
it('should revert if the hash of the pre-image data and dataHash do not match for EIP-1271 signature', 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 randomBytes = crypto.pseudoRandomBytes(128).toString('hex')
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)
])
const randomHash = `0x${crypto.pseudoRandomBytes(32).toString('hex')}`
const randomBytes = `0x${crypto.pseudoRandomBytes(128).toString('hex')}`
const randomAddress = `0x${crypto.pseudoRandomBytes(20).toString('hex')}`
const randomSignature = `0x${crypto.pseudoRandomBytes(65).toString('hex')}`

const eip1271Sig = buildContractSignature(randomAddress, randomSignature)
const signatures = buildSignatureBytes([eip1271Sig])
await expect(
safe.checkNSignatures(txHash, randomBytes, signatures, 3)
safe.checkNSignatures(randomHash, randomBytes, signatures, 1)
).to.be.revertedWith("GS027")
})
})
Expand Down

0 comments on commit 981c8ff

Please sign in to comment.