Transfers / Sends can increase calling account's balance
cgewecke opened this issue · 6 comments
Jim Berry reported this discovery via slack.
It's a bug introduced by the fix for #106. There we increased the vm's callStipend value (a lot) to prevent send throwing when the fallback function of the target is instrumented. A side effect of this is that the excess stipend gets refunded to the account calling the method. This doesn't affect contract to contract sends (i.e the contracts' balances aren't modified).
Have looked through the vm and don't see a simple resolution. That doesn't mean there isn't one!
Question: should we revert? Is it more common for people to instrument their fallback functions than test calling account balances after a send? The original bug was very confusing - it triggered an invalid opcode within a bunch of nested functions. On the other hand sending but ending up with a higher balance than expected is disturbing....
Leaving this open for comment.
A quick fix for anyone who has the balance problem is to use solidity-coverage@0.2.2 and add wallet contracts (or any send/transfer recipient contracts) to the skipFiles option in .solcover.js. If you're just sending to one of the commonly used wallets you're probably not writing tests for those and don't need them covered. . .
Another quick fix is to wrap the weirdly failing balance check assertions in a try/catch block that suppresses the throw if you've set an environment variable like SOLIDITY_COVERAGE to true. And launch the tool like:
SOLIDITY_COVERAGE=true ./node_modules/.bin/solidity-coverage
Stipend is there for a reason - to prevent calling complicated code via send which may result in re-entry into the sender. Fallback function intended to be called via send should be extremely simple (like log smth, stipend prevents any changes to state). Still fallback function should be instrumented. It's used for example in ICO contracts and intended to be called with large gas limit externally.
I'd say just ignore this case and rollback #106. Testing balances is extremely hard now and requires assuming that balances are rising due to code execution;>
and thx for a great tool!
@rudolfix Thanks so much. Yes I think this corruption of the caller balances is unacceptable too.
Increasing the balance definitely feels worse than having the fallback function fail.
Ok, at this point it will require 5 new votes to keep stipend increase. So we'll stipexit today and make a note in the FAQ somewhere about this.
Reverted in 0.2.5