Heard Back from Safe Team's Lead Dev: Reason to be ConcernedThe Safe contracts are vulnerable. Undeniably. Hashing an EIP-712 hash
into the EIP-191 standard clearly breaks the EIP-191 standard and violates every property of EIP-712 due to the simple fact we can convert it into an EIP-191.
The Safe EIP-191 implementation is not compliant with
any wallet that's ever been made in mankind. Ever.
What determines whether an EIP-712 transaction hash is nested into an EIP-191 personal sign is the
v byte of the r,s,v signature split (for each signature). If that value is > 30 (in hexadecimal), the contract takes the EIP-712 transaction hash the signatures would have otherwise been verified against and prefixes that with an EIP-191 prefix, then keccak256 hashes that result and verifies the signatures against that.
You know what's supposed to happen to the EIP-712 transaction hash?
Nothing. This also violates an invariant in all versions of the Safe contract which dictates there shouldn't be any state changes during the
checkSignatures function. However, with
consumeHash = 1, and that hash mutating (bug),
this constitutes as a state change in violation of the Safe contract's logic.
Attack Vector Okay, I'm North Korea. I want to hack you Westerners. I focus on a very wealthy proxy that's deployed under Safe's buggy contracts (let's say, Bybit).
I
don't need to get the owners to directly sign a malicious transaction. At all. If I can create a malicious transaction (H)M, that when erroneously double hashed and nested in the EIP-191 standard yields an output (H)M' that is equivalent to a real transaction hash (H)R, then I'm in there like swim wear.
So
(H)M' = (H)R is the goal for an attacker.
At first glance, this seems impossible right? But not really.
I showed the
calldata for the
execTransaction function call a few posts back. You probably noticed huge blocks of repetitious zeroes. That's how most of those function calls look. The only real difference from call to call might be the
to,
value and the
data parameters - that's it. Rest of the values will pretty much be the same.
Why's that relevant? That limits the "wiggle room" there is for variability in these transactions (entropy) - which, effectively,
reduces the collision space an attacker needs to search. Exponentially (literally). Further consider that Ethereum addresses themselves are already truncated to a 20-byte keccak256 hash of their public key (
keccak256 hash function spits out 32-byte outputs).
Addresses are also encoded (restricting character choice to certain letters and all numbers that must adhere to a unique sub-grammar). The
operation parameter is just a
uint8. The nested
transfer has an identical prefix (`0xa9059cbb`), different "token address" with no token amount (effectively more null bytes).
Here's one instance on the blockchain where the only difference between these two (legitimate)
transactions (besides the fact one failed & the other didn't)
was the nonce value: 1.
0x5bd338eb788898addcbd079cac3e661ae139f0b3b5fb354f83012a438da88b712.
0xd20a076a43804ffd392587cfb3a1a8825a482b768c2184beffef71b566c29ed6 That's just a
1-byte difference.
Thanks to this bug, an attacker can aggregate every valid signature that's ever been produced by the victim's group of authorized signers (`EIP-712`on any proxy at any version on Ethereum at least) with an
execTransaction function and attempt to "mine" a malicious transaction with parameters that, when erroneously nested in the EIP-191 standard, results in a hash that collides with a legitimately signed TX (wherever it can be found).
When considering the degrees of freedom the attacker has (in crafting a colliding transaction), dozens or hundreds of valid candidates (`H(R_i)`). Which effectively resolves to
H(M)' ∈ {H(R_i), H(R_ii)...H(R_n) this is practical as fuck.
