roguereddwarf - HatsSignerGateBase: valid signer threshold can be bypassed because HSG checks signatures differently from Safe which allows exploitation #50
HatsSignerGateBase: valid signer threshold can be bypassed because HSG checks signatures differently from Safe which allows exploitation
Summary
This report deals with how the HatsSignerGate and the Safe check signatures differently which opens the door to exploitation.
I will show how this allows a valid signer that has become invalid but not yet removed from the owners of the Safe to continue signing transactions. The invalid signer can effectively sign transactions as though he was valid.
Also there is the possibility of valid signers calling Safe.addOwnerWithThreshold. When an owner is added to the Safe but not a valid signer he can still sign transactions and the HSG will not recognize that there are not enough valid signatures.
To summarize, the issue is caused by this:
Signatures are first checked by the Safe then by the HSG logic
We can pass an arbitrary amount of signatures when executing a transaction
The Safe checks that the first threshold signatures are valid. However the HSG logic checks that ANY of the signatures are signed by valid signers. The HSG logic does not check the same signatures as the Safe.
Essentially the Safe and HSG logic are applying different checks to different signatures.
First the signatures are checked by the Safe Link then the checkTransaction function is executed on the guard (HatsSignerGate) Link
The HatsSignerGate then executes countValidSignatures to check if enough signatures were signed by valid signers Link
With all prerequisites out of the way, we can now get into the actual issue.
The Safe calls checkNSignatures to check if the first threshold signatures in the signatures bytes are valid Link
So if threshold=5 but we provide say 7 signatures, the last two signatures are not checked.
If the first 5 signatures are valid the check passes successfully.
The issue is that the HSG countValidSignatures function iterates over ALL signatures and tries to find enough valid signers such that threshold is reached Link.
So imagine the following scenario:
There are 4 owners in the Safe, threshold=3 and 3 owners are valid signers.
One of the owners is no longer a valid signer (I'll call him Bob). He is not yet removed from the owners.
Bob wants to sign a transaction and submit it to the Safe. He already has 2 signatures from valid signers.
Bob signs the transaction and appends his signature to the signatures bytes. He also appends a signature of a valid signer from a previous transaction. So there are now 4 signatures in the signatures bytes.
Bob calls Safe.execTransaction. The Safe checks the first 3 signatures to be valid signatures from owners. The check passes. The HSG checks that there are at least 3 signatures signed by valid signers. Which also passes.
To summarize: Bob was able to sign a transaction even though he was no longer a valid signer.
Further notes
Another thing to note is that HSG does not check signatures for uniqueness so if Bob would have to append multiple signatures from valid signers he could just add the same signature multiple times.
Also the HSG does not check that ecrecover does not return the zero address as owner which it does if the signature is invalid.
These checks are implemented in the Safe. So by implementing the mitigation I suggest below the Safe and HSG will check the same signatures. So there is no need to have these checks in the HSG as well.
However due to this bug (checking different signatures), the signer hat might be transferred to address(0) which then causes invalid signatures to be considered valid.
Impact
Owners of the Safe that are not valid signers can sign transactions.
Code Snippet
Also have a look at the @audit-info comments that further explain the issue.
function execTransaction(
address to,
uint256 value,
bytes calldata data,
Enum.Operation operation,
uint256 safeTxGas,
uint256 baseGas,
uint256 gasPrice,
address gasToken,
address payable refundReceiver,
bytes memory signatures
) public payable virtual returns (bool success) {
bytes32 txHash;
// Use scope here to limit variable lifetime and prevent `stack too deep` errors
{
bytes memory txHashData = encodeTransactionData(
// Transaction info
to,
value,
data,
operation,
safeTxGas,
// Payment info
baseGas,
gasPrice,
gasToken,
refundReceiver,
// Signature info
nonce
);
// Increase nonce and execute transaction.
nonce++;
txHash = keccak256(txHashData);
// @audit-info first the Safe checks the signatures
checkSignatures(txHash, txHashData, signatures);
}
address guard = getGuard();
{
if (guard != address(0)) {
// @audit-info then signatures are checked by HSG
Guard(guard).checkTransaction(
// Transaction info
to,
value,
data,
operation,
safeTxGas,
// Payment info
baseGas,
gasPrice,
gasToken,
refundReceiver,
// Signature info
signatures,
msg.sender
);
}
}
// We require some gas to emit the events (at least 2500) after the execution and some to perform code until the execution (500)
// We also include the 1/64 in the check that is not send along with a call to counteract potential shortings because of EIP-150
require(gasleft() >= ((safeTxGas * 64) / 63).max(safeTxGas + 2500) + 500, "GS010");
// Use scope here to limit variable lifetime and prevent `stack too deep` errors
{
uint256 gasUsed = gasleft();
// If the gasPrice is 0 we assume that nearly all available gas can be used (it is always more than safeTxGas)
// We only substract 2500 (compared to the 3000 before) to ensure that the amount passed is still higher than safeTxGas
success = execute(to, value, data, operation, gasPrice == 0 ? (gasleft() - 2500) : safeTxGas);
gasUsed = gasUsed.sub(gasleft());
// If no safeTxGas and no gasPrice was set (e.g. both are 0), then the internal tx is required to be successful
// This makes it possible to use `estimateGas` without issues, as it searches for the minimum gas where the tx doesn't revert
require(success || safeTxGas != 0 || gasPrice != 0, "GS013");
// We transfer the calculated tx costs to the tx.origin to avoid sending it to intermediate contracts that have made calls
uint256 payment = 0;
if (gasPrice > 0) {
payment = handlePayment(gasUsed, baseGas, gasPrice, gasToken, refundReceiver);
}
if (success) emit ExecutionSuccess(txHash, payment);
else emit ExecutionFailure(txHash, payment);
}
{
if (guard != address(0)) {
Guard(guard).checkAfterExecution(txHash, success);
}
}
}
// @audit-info requiredSignatures is equal to threshold
function checkNSignatures(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.
address lastOwner = address(0);
address currentOwner;
uint8 v;
bytes32 r;
bytes32 s;
uint256 i;
// @audit-info only the first threshold signatures are checked
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)));
// 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
require(uint256(s) >= requiredSignatures.mul(65), "GS021");
// Check that signature data pointer (s) is in bounds (points to the length of data -> 32 bytes)
require(uint256(s).add(32) <= signatures.length, "GS022");
// Check if the contract signature is in bounds: start of data is s + 32 and end is start + signature length
uint256 contractSignatureLen;
// solhint-disable-next-line no-inline-assembly
assembly {
contractSignatureLen := mload(add(add(signatures, s), 0x20))
}
require(uint256(s).add(32).add(contractSignatureLen) <= signatures.length, "GS023");
// Check signature
bytes memory contractSignature;
// solhint-disable-next-line no-inline-assembly
assembly {
// 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)
}
require(ISignatureValidator(currentOwner).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "GS024");
} 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
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");
} 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
currentOwner = ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s);
} else {
// Default is the ecrecover flow with the provided data hash
// Use ecrecover with the messageHash for EOA signatures
currentOwner = ecrecover(dataHash, v, r, s);
}
require(currentOwner > lastOwner && owners[currentOwner] != address(0) && currentOwner != SENTINEL_OWNERS, "GS026");
lastOwner = currentOwner;
}
}
function countValidSignatures(bytes32 dataHash, bytes memory signatures, uint256 sigCount)
public
view
returns (uint256 validSigCount)
{
// There cannot be an owner with address 0.
address currentOwner;
uint8 v;
bytes32 r;
bytes32 s;
uint256 i;
// @audit-info all signatures are checked
for (i; i < sigCount;) {
(v, r, s) = signatureSplit(signatures, i);
// @audit-info old signature is counted as valid because transaction data is not verified
if (v == 0) {
// 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)));
// @audit-info old signature is counted as valid because transaction data is not verified
} 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
currentOwner = address(uint160(uint256(r)));
} 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
currentOwner =
ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s);
} else {
// Default is the ecrecover flow with the provided data hash
// Use ecrecover with the messageHash for EOA signatures
currentOwner = ecrecover(dataHash, v, r, s);
}
if (isValidSigner(currentOwner)) {
// shouldn't overflow given reasonable sigCount
unchecked {
++validSigCount;
}
}
// shouldn't overflow given reasonable sigCount
unchecked {
++i;
}
}
}
Tool used
Manual Review
Recommendation
I propose that in the HatsSignerGate only the first threshold signatures are checked. Such that both the Safe and HSG check the SAME signatures.
Instead of checking all signatures, only the first threshold ones will be checked.
Also there is no need to check the length of the signatures bytes. All those checks are done by the Safe already.
roguereddwarf
high
HatsSignerGateBase: valid signer threshold can be bypassed because HSG checks signatures differently from Safe which allows exploitation
Summary
This report deals with how the
HatsSignerGate
and theSafe
check signatures differently which opens the door to exploitation.I will show how this allows a valid signer that has become invalid but not yet removed from the
owners
of the Safe to continue signing transactions. The invalid signer can effectively sign transactions as though he was valid.Also there is the possibility of valid signers calling
Safe.addOwnerWithThreshold
. When an owner is added to the Safe but not a valid signer he can still sign transactions and the HSG will not recognize that there are not enough valid signatures.To summarize, the issue is caused by this:
threshold
signatures are valid. However the HSG logic checks that ANY of the signatures are signed by valid signers. The HSG logic does not check the same signatures as the Safe.Essentially the Safe and HSG logic are applying different checks to different signatures.
Vulnerability Detail
A transaction is executed by calling Safe.execTransaction
First the signatures are checked by the Safe Link then the
checkTransaction
function is executed on the guard (HatsSignerGate) LinkThe HatsSignerGate then executes
countValidSignatures
to check if enough signatures were signed by valid signers LinkWith all prerequisites out of the way, we can now get into the actual issue.
The Safe calls
checkNSignatures
to check if the firstthreshold
signatures in thesignatures
bytes are valid LinkSo if
threshold=5
but we provide say 7 signatures, the last two signatures are not checked. If the first 5 signatures are valid the check passes successfully.The issue is that the HSG
countValidSignatures
function iterates over ALL signatures and tries to find enough valid signers such thatthreshold
is reached Link.So imagine the following scenario:
threshold=3
and 3 owners are valid signers.owners
.signatures
bytes. He also appends a signature of a valid signer from a previous transaction. So there are now 4 signatures in thesignatures
bytes.Safe.execTransaction
. The Safe checks the first 3 signatures to be valid signatures from owners. The check passes. The HSG checks that there are at least 3 signatures signed by valid signers. Which also passes.Important: Bob can pass a 4th signature from a previous transaction because two of the signature types used in HSG do not check that the correct data has been signed https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L561-L568.
To summarize: Bob was able to sign a transaction even though he was no longer a valid signer.
Further notes
Another thing to note is that HSG does not check signatures for uniqueness so if Bob would have to append multiple signatures from valid signers he could just add the same signature multiple times.
Also the HSG does not check that
ecrecover
does not return the zero address as owner which it does if the signature is invalid. These checks are implemented in the Safe. So by implementing the mitigation I suggest below the Safe and HSG will check the same signatures. So there is no need to have these checks in the HSG as well. However due to this bug (checking different signatures), the signer hat might be transferred toaddress(0)
which then causes invalid signatures to be considered valid.Impact
Owners of the Safe that are not valid signers can sign transactions.
Code Snippet
Also have a look at the
@audit-info
comments that further explain the issue.https://github.com/safe-global/safe-contracts/blob/cb22537c89ea4187f4ad141ab2e1abf15b27416b/contracts/Safe.sol#L135-L217
https://github.com/safe-global/safe-contracts/blob/cb22537c89ea4187f4ad141ab2e1abf15b27416b/contracts/Safe.sol#L270-L330
https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L445-L503
https://github.com/Hats-Protocol/hats-zodiac/blob/9455cc0957762f5dbbd8e62063d970199109b977/src/HatsSignerGateBase.sol#L547-L591
Tool used
Manual Review
Recommendation
I propose that in the HatsSignerGate only the first
threshold
signatures are checked. Such that both the Safe and HSG check the SAME signatures.Fix:
Instead of checking all signatures, only the first
threshold
ones will be checked. Also there is no need to check the length of thesignatures
bytes. All those checks are done by the Safe already.