Underhanded coding contests are a good way to highlight shortcomings of a programming language. Solidity has a big advantage here over languages like C, because it can still be changed to some degree without breaking too many tools and programs. Because of that, in reaction to the Underhanded Solidity Contest (results) I would like to provide an analysis of some of the submissions and explain how we changed the language (or plan to change it) or at least explain which compiler warnings we introduced.
Shadowing
Many of the submission exploited the fact that it was possible to shadow built-ins like now or msg. We already added warnings in such situations shortly before the beginning of the contest. The solution is not yet complete, because there are still some ways to confuse users by shadowing/overloading using inheritance, but we are also working on that.
Bypassing Ether Accounting
Another very common theme was to use the fact that it is possible to send Ether to a contract without triggering the fallback function, and thus bypassing any internal accounting that might be done there.
Three example submissions that exploit this are the one by João Carvalho (won third prize), Richard Moore (won second prize) and Christoph Jentzsch (a very short example). This is a quite tricky problem to tackle by means of the language, because sending Ether without executing the receiving contract cannot be prevented without serious changes to the Ethereum protocol. We could introduce a “magic storage variable” that automatically tracks Ether sent via regular means, but that would be quite expensive and because of that would also make it impossible for a contract to receive Ether within the fallback function gas stipend. Another measure would be to warn about this issue whenever this.balance is used outside of the context of a mere x.transfer(this.balance) , but it would probably have a quite high false positive rate. This problem has been part of the Security Considerations section of the Solidity documentation for a long time, and at the moment, this is the best “solution” we could come up with.
Gigantic Arrays
A different problem is exploited in the submission by Doug (honorable mention). It allows overwriting objects in storage by generating a gigantic array.
The following two problems lead to the attack:
- The
lengthmember of storage arrays can be assigned arbitrarily. - Gigantic storage arrays (as opposed to mappings) overlap predictively.
Since changing the storage layout would be a quite drastic change which would probably also make it necessary to add version information to libraries, it is better to fix point 1.
With the introduction of the SMT-based overflow checker, the underflow used in the exploit would be noticed at compile-time and it would be required to change the code from
to
to silence the warning and thus also make the exploit impossible.
The introduction of a pop method, as suggested in the code, would of course also help.
This still does not solve situations where you use something like x.length = 2**200 directly. While Solidity currently warns you about such large arrays if they are static, it does not do so for dynamic arrays. Here, the SMT solver might come to a rescue and require that such assignments to length are either “small” or they are done using x.length += a or x.length -= a for a “small” a .
Overflows in the ABI Decoder
The winning entry by Martin Swende exploited an overflow in the ABI decoder. The attacker would craft a special transaction payload such that the length property of an array is gigantic while the data used is comparatively small. The second fact is required because the transaction would require too much gas otherwise. This is made possible because the ABI decoder routine that computes the number of bytes from the array length (size_in_bytes = number_of_elements * 32) overflows. Still, it is quite hard to exploit this and the smart contract does it by using the explicit length member in one part of the code and another length that was computed manually from the size of the calldata in another part of the code. The resulting code looks a little weird as expected.
Since the bug is hard to exploit and we are working on a re-implementation of the ABI encoder and decoder in the new intermediate language iulia anyway, we decided to only put safety measures into the new decoder. Apart from avoiding such overflows, the new decoder also checks that it never reads beyond the supplied data length (which is actually fine because it is specified to be filled with zeros, but indicates an invalid input encoding).
Miscellaneous
The submission by glockenspieleth relies on type deduction rules which might be counter-intuitive at first, especially in the well-known snippet for (var i = 0; i < x.length; x++) , but this case has been warned about in the compiler for quite some time now.
A submission by andrejcvoro exploited another bug in a for loop:
for (uint i = 4; i >= 0; i--) { ... }
This loop will not terminate because i >= 0 is always true. Such cases will be detected by the SMT solver in the future.
The contract by blockie exploits the unary plus infamous due to a bug in the hacker gold token contract. The compiler warns about this for quite some time and it will be fully removed in the next breaking release.
Marco Giglio exploits homoglyphs (different characters that look similar or identical) in string literals. We currently do not plan any countermeasure against this since there is very little use for string literals in Solidity (as opposed to e.g. JavaScript). The fact that the contract uses string literals and not type-safe structs is hopefully suspicious enough. If you think otherwise, please get in touch.
Uninitialized storage pointers are exploited by sammayo, which leads to unintended overwrites in storage. The compiler has a warning for the usual cases for a long time, and we are working to catch the remaining special cases. It now also requires an explicit storage keyword for storage pointers.
The submission by Zachary Williamson uses — among other things — the fact that due to the way the EVM is built, the contract cannot call itself at the point of its construction. A warning about the most common case has been added some months ago and the static analyzer in remix already warns about that for even longer.
Get in Touch
If you found a bug in the code generation part of Solidity, please report it to the Ethereum Foundation’s bug bounty program. Please avoid prior public disclosure in order to be eligible for a reward.
For not so serious issues, please open an issue in our issue tracker or talk to us on gitter.im/ethereum/solidity-dev.
If you want to implement security measures, please also either talk to us or (for smaller things) just open a pull request. If you want to make a long-term commitment to developing Solidity and would like to be paid for that, please consider applying for a job at solidity-jobs@ethereum.org.
