eth/harness: Uncomment init overflow test. #1722
Conversation
|
I think @martonp wrote the test, and I commented it out, because it was failing after a refactor, and I though there was no way to actually overflow with the new architecture. I'm not fully caught up on this convo, but from what I gather, I was wrong? Anyway, more tests and better security is always welcome. |
|
Yah, good to check these things anyway. But why is the contract bin changing? |
|
|
||
| // pragma should be as specific as possible to allow easier validation. | ||
| // | ||
| // WARNING: pragma MUST be above v0.8.0 so that addition overflow will cause | ||
| // transactions to be reverted. |
There was a problem hiding this comment.
If changing the comments really changes the bin, let's just skip this. I don't think we'll ever consider downgrading to before 0.8.0.
There was a problem hiding this comment.
So comments really change the output? That's kinda nuts
There was a problem hiding this comment.
I guess there's a hash of the entire thing and a checksum somewhere. I'm guessing it's only a checksum changing. Can check how much changes because that's good info to have.
There was a problem hiding this comment.
At the end of the hex, this part changed:
9c8d8d137b7639af24fe8215dd5976b16323176cb4b9be076c90eeb32eaf7446 -> ef281bf04bfe702e408f8eba5336858e62259885e954f238248ad567172b2764
So, a 32 byte hash, probably of the entire contract including comments.
There was a problem hiding this comment.
Probably. Geeze it should be a hash of a language preprocessor's output (or optionally omitted), not the file contents. Oh well
There was a problem hiding this comment.
No, just wanted to make sure. The contract is fine afaik. |
JoeGruffins
force-pushed
the
overflowinit
branch
from
c158bd4
to
15e4e83
Compare
There could also be a change reverting the behavior in the future. However unlikely that may be. |
closes #1720
So, there was already a test! I guess @buck54321 or @martonp noticed the possible vulnerability before me. The commented out test just never registered for me....
Anyway, I tried this with a contract build with 0.7.5 and can confirm the init succeeds and test fails.