>It turns out that during a routine upgrade, the Nomad team initialized the trusted root to be 0x00. To be clear, using zero values as initialization values is a common practice. Unfortunately, in this case it had a tiny side effect of auto-proving every message
EDIT:
Reading and noodling I'm 99% sure these are separate issues. The vulnerability talks about passing in an empty leaf to the prove function. But that's not what the exploit is.
The exploit is using an unproven message. So they are passing in an actual leaf to prove. The problem is that unproven messages have 0x00 as root and some jabroni set 0x00 as the trusted root. So every message was treated as proven by default when it should be the opposite.
Not a hard and fast rule, and not something that will catch tons of problems, but now and again it does help me catch an uninitialized value.
then whoever takes over maintenance from you and wants to make improvements says "hey, we can add this new v2 feature through the 0x0 which is available for future enhancements"
enum class Color {
Invalid, // Not a valid value.
Red,
Orange,
Yellow,
Green,
};
I think if someone's crazy enough to reclaim a 0 value in an enum or integer ID, then they're probably going to do a lot of damage to the code no matter what you do.Fun part is as far as I understand this is just a convention and there is nothing intrinsic to 0x0 that makes it different from any other destination address. If someone was to find the private key for which 0x0 is the public half they would have access to a vast amount of stuff that has been transferred there over time. Finding this key is computationally unfeasible however.
yes, a routine upgrade. that's what it was..... (→_→)
Very unhappy with how Celo handled the situation, as was much of the technical crypto community at the time.
The real issue was half-caught in a review on a pull request however. https://github.com/nomad-xyz/monorepo/pull/289/files
If this legacy enum value had been handled later in the code, there would not have been a vulnerability.
(This isn't to say that the developers were bad. The person who wrote the code was extremely knowledgeable. It's just really hard to be perfect every time. )
They are bad because they are not competent to write the decent code required by their profession and job environment.
In normal software writing trade, such engineers are called low performers and routinely managed out of any organization.
Sure, the mistake is not unusual from the perspective of general software engineering. But let's not forget what software they are working on.
I am totally fine with a bartender dropping a glass... I'll put a surgeon on trial if he cannot make his hands steady during a heart surgery...
I remember AWS S3 went down in 2017 or so and the key point I took away from their article about it was that they didn't blame the junior dev that caused it, because it shouldn't have been able to happen in the first place.
Ah here we are: https://aws.amazon.com/message/41926/ "We are making several changes as a result of this operational event..." basically boiling down to "the employee in question is not at fault because our tools should not have let him do that".
This "you must be perfect" mentality is detrimental to building a security culture, IMO — no one is perfect, and the most excellent dev will slip up. Seeing the people involved in that commit, I believe that's what happened here.
Hoping we learn more in the post-mortem, and they revise their practices to catch this mistake in the future.
> QSP-19 Proving With An Empty Leaf
> Recommendation: Validate that the input of the function is not empty
> The Nomad team responded that "We consider it to be effectively impossible to find the preimage of the empty leaf".
> We believe the Nomad team has misunderstood the issue. It is not related to finding the pre-image of the empty bytes. Instead, it is about being able to prove that empty bytes are included in the tree (empty bytes are the default nodes of a sparse Merkle tree). Therefore, anyone can call the function with an empty leaf and update the status to be proven.