search

nevillegrech / MadMax

Ethereum Static Vulnerability Detector for Gas-Focussed Vulnerabilities
BSD 3-Clause "New" or "Revised" License
130 stars 16 forks source link

ecrecover related bugs are not detected #1

Closed HarryR closed 3 years ago

HarryR commented 5 years ago

Description

It doesn't detect ecrecover failing upon invalid input

How to Reproduce

See the following piece of code:

https://gist.github.com/HarryR/cce52596ffebdff2744c5d790888015a

This was caused by a compiler bug in Solidity < 0.4.14, where the output memory area for the ecrecover call wasn't cleared, which means in the case of an invalid signature the memory may contain user-controllable input.

If the contract address is passed in as the last 20 bytes of the 32-byte stuff2hash input, then the if condition will be true and the contract will send all funds to the caller.

This was recently highlighted as a problem with the 0x contracts, see: https://samczsun.com/the-0x-vulnerability-explained/

Expected behavior

this bug should be detected

nevillegrech commented 5 years ago

Hi @HarryR. Thanks for letting us know about this vulnerability variant. I was well aware of the 0x vulnerability but the one you're describing is a bit different (but similar behavior). In the 0x case, it wasn't a compiler bug but an asm STATICCALL that did not use RETURNDATASIZE or cleared the memory that was allocated for return data.

Is the bug you describe only applicable to ecrecover, or to all precompiles? Also, what other characteristics do these vulnerable smart contracts have. We could easily flag all smart contracts with precompiles. There's around 50k on the mainnet with precompiles 1 to 3, but this wouldn't be useful.

HarryR commented 5 years ago

I agree, flagging all contracts that use precompiles isn't very useful, at that point you're not really reasoning about the control flow of a contract. I really think proper support for precompiles is necessary, even if it's just at a semantic level.

The general bug above exists for any precompile which may return an error and not overwrite the output range, but the compiler bug indicated a trend of forgetting that functions may error, I think the ecmul and ecadd precompiles may do this too, in addition to ecrecover.

The identity precompile simply copies memory in a more efficient way, and most of the tools seem to fail with that too.

nevillegrech commented 4 years ago

I think I have implemented this check on contract-library.com. Can you see whether this flags the contracts you had in mind?

Here's a list of contracts that get flagged by this: https://contract-library.com/?w=Unchecked%20Tainted%20staticcall