Consensys / mythril

Security analysis tool for EVM bytecode. Supports smart contracts built for Ethereum, Hedera, Quorum, Vechain, Rootstock, Tron and other EVM-compatible blockchains.
https://mythx.io/
MIT License
3.84k stars 736 forks source link

Feature request : please add Wapped Ether/ᴡᴇᴛʜ token support to the ether_thief.py module ! #1279

Open ytrezq opened 4 years ago

ytrezq commented 4 years ago

More and more contracts choose to use the de‑facto wrapper instead of handling directly even if they don’t handle other tokens. Exchange can then be performed on‑chain 1 to 1 for free without any restrictions, so essentially this is exactly the same as Ether.

Instead of only checking ether sending, please also include an ᴇʀᴄ20 check of token send using 0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2 too. This would add the benefit of working faster (because the possibility to withdraw from the contract requires a subsequent transaction and might not be found because of transaction number limit with -t parameter or the planned recursion limit parameter) or even trigger the module which wouldn’t be triggered otherwise.

An alternative would be to treat 0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2 as a precompile instead of calling it since as far I’m aware, it’s perfectly and definitely bug‑free which means searching bugs inside it is pointless.

ytrezq commented 4 years ago

Isn’t the pre_hook which should be modified ? if yes, how to add not just one instruction, but a transaction sequence ?

norhh commented 4 years ago

Something similar to this is being done by @nbanmp I think. But it's a bit more generic.

ytrezq commented 4 years ago

@norhh : but with which issue number ?

norhh commented 4 years ago

It's tracked in a private board as it is related to MythX as a whole.

ytrezq commented 4 years ago

@norhh : so no way to get more information about it ?

norhh commented 4 years ago

@nbanmp can fill you in on it.

nbanmp commented 4 years ago

This is an interesting idea!

I was working on coming up with a good way to write modules to analyze and detect vulnerabilities in ERC20 contracts. I think what you're suggesting only has a slight overlap with my work on ERC20 analysis.

You're suggesting that we essentially assume the token contract is secure, and we try to detect contracts that voluntarily send more of a token then they receive. That is likely slightly easier, but solves a different problem, as it doesn't need to check functional properties of the contract.

The challenge would be symbolically modeling token balances and essentially mocking an erc20 contract. This could probably be abstracted to work with other erc20 tokens as well.

ytrezq commented 4 years ago

@nbanmp : no challenge here. Wrapped Ether is Ether. So either they are received through transferfrom in the contract or plain send or they are sent through the contract or plain send. That’s why I’m suggesting to just add it to the Ether_thief.py module (though this wouldn’t remove its analysis).

This might also remove a false positive where a contract/game receive transferfrom Ether and send them through plain send. Something your ᴇʀᴄ20 work won’t solve.

The contract code is pretty straightforward, and Mythril finds nothing when running on it. With 2 millions Ethers held for years, I’m interested if there is a vulnerability inside it (not only there’s no vulnerability but it’s likely bug free).

ytrezq commented 4 years ago

@nbanmp : is #1371 the broader case you’re working on?

ytrezq commented 7 months ago

@nbanmp : any update on this?