Dry Garnet Cobra
High
A single valid signature can be repeated multiple times to bypass the threshold.
To verify that a transaction can be executed, a sufficient number of signers must have signed the transaction hash:
// get the tx hash
bytes32 txHash = safe.getTransactionHash(
to,
value,
data,
operation,
safeTxGas,
baseGas,
gasPrice,
gasToken,
refundReceiver,
// We subtract 1 since nonce was just incremented in the parent function call
safe.nonce() - 1
);
// count the number of valid signatures and revert if there aren't enough
if (_countValidSignatures(txHash, signatures, threshold) < threshold) revert InsufficientValidSignatures();Digging into _countValidSignatures:
function _countValidSignatures(bytes32 dataHash, bytes memory signatures, uint256 sigCount)
internal
view
returns (uint256 validSigCount)
{
// There cannot be an owner with address 0.
address currentOwner;
uint8 v;
bytes32 r;
bytes32 s;
uint256 i;
for (i; i < sigCount; ++i) {
(v, r, s) = signatureSplit(signatures, i);
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)));
} 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;
}
}
}
}Here, we see that _countValidSignatures iterates through the signatures data to determine the address of each signer, and will verify whether they are valid using a call to isValidSigner:
function isValidSigner(address _account) public view returns (bool valid) {
/// @dev existing `registeredSignerHats` are always valid, since `_validSignerHats` is append-only
/// We don't need a special case for `_account == address(0)` because the 0 hat id does not exist
valid = HATS.isWearerOfHat(_account, registeredSignerHats[_account]);
}To summarize:
- The hash of the transaction intended to be executed is signed by a number of signers to create
signatures. - The
signaturesare verified to check that each individual signer corresponds to the address of a valid signer, and they have authorized the transaction hash. - The number of valid signer addresses are counted, and if this count exceeds the execution threshold, the transaction is executed.
Now, consider the case where the signatures consists of a valid signature repeated multiple times.
Since this logic fails to ignore signatures from addresses that have already contributed to the validSigCount, a single valid signature can be repeated multiple times to exceed the threshold for execution.
No response
- A valid signer for the aafe exists (either a traditional owner or hat wearer).
- A single valid signer creates a valid signature for a transaction.
- The signer maliciously duplicates the valid signature multiple times until they exceed the execution threshold for the safe.
- The signer submits the transaction.
A single signer can forcibly execute transactions from the safe, irrespective of the execution threshold.
No response
Enforce that the array of signatures to be specified in ascending order of addresses, and make sure that the currentOwner for each signature is greater than the previous:
function _countValidSignatures(bytes32 dataHash, bytes memory signatures, uint256 sigCount)
internal
view
returns (uint256 validSigCount)
{
// There cannot be an owner with address 0.
address currentOwner;
+ address lastOwner;
uint8 v;
bytes32 r;
bytes32 s;
uint256 i;
for (i; i < sigCount; ++i) {
(v, r, s) = signatureSplit(signatures, i);
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)));
} 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);
}
+
+ /// @notice prevent sybil attacks
+ require(currentOwner > lastOwner);
+ lastOwner = currentOwner;
if (isValidSigner(currentOwner)) {
// shouldn't overflow given reasonable sigCount
unchecked {
++validSigCount;
}
}
}
}This will also prevent against the ecrecover signature malleability vulnerability currently present in the implementation of _countValidSignatures.