After thorough assessment of all submissions, we are happy to share the winners of this yearâs Underhanded Solidity Contest!
If you are not familiar with it, please read the announcement from September.
Before we dive into the winning submissions, weâd like to thank all participants for taking part. In total, we received 16 qualifying submissions which you can find in this repo. All 16 submissions are eligible for a âqualified submissionâ Underhanded Solidity POAP NFT - winners will receive an additional âWinnersâ POAP NFT.
We will be in touch with all participants shortly with details on the claiming process for the NFTs as well as the main prizes.
Now, letâs have a look at this yearâs winners, starting from the 5th place!
Underhanded Solidity Contest 2020 Winners
5ď¸âŁ th Place: Marius van der Wijden
commentary by Alex Beregszaszi
This submission highlights the need for clearly distinguishing error conditions from return values. The ecrecover
precompile has a single output, the address, which is set to zero in case of errors. While some high-level wrappers for this precompile made a distinction, many introduce new error conditions, but still reuse the same mechanic of returning a âzero addressâ on error. This was the case with OpenZeppelinâs version prior to the 3.0 series.
Using this validation shortcoming, the submission allows anyone to create unlimited âvote acceptedâ transactions.
Solidity has always supported multiple return values which allows returning a status/error code and data separately, but more importantly the established best practice is to use require
statements for validation, which most ecrecover
use cases follow today.
While the submission is written clearly and the issue is not obvious without dissecting both the closeVote
and the trusted ECDSA.recover
functions, in our opinion there are two shortcomings raising suspicion:
- Append only data structures are not commonly used.
- Awareness about
ECDSA.recover
was raised recently, and thus one would expect a more thorough review of its applications. The submission used the version from OpenZeppelin 2.5.0.
4ď¸âŁ th Place: Richard Moore
commentary by Stefan Beyer
The submission exploits CREATE2
to hijack a multisig contract that controls upgradability. Whilst this is a known issue, the scenario demonstrates the impact of CREATE2
in combination with self-destruct and shows the importance of assessing the contracts deployment procedure in a security analysis.
3ď¸âŁ rd Place: Cory Dickson
commentary by Goncalo SĂĄ
This submission highlights the perils of validating non-EOA (Externally Owned Accounts) addresses within the EVM. Even though assessing whether an account interacting with a certain system is not a contract has been the source of many production hacks on mainnet and therefore is a well-studied, bad-practice pattern, the one where a user âDoS-esâ a system by self-destructing their contract is not.
In a nutshell, this attack demonstrates the possibility of a lock-up scenario in a governance system that depends on composability and expects the existence of a smart contract at a certain address to be a time-independent function. Since the OpenZeppelin library used to check if an account is a contract or not returns true even though a contract might self-destruct at the end of a transaction, the attacker is able to lock-up following governance votes to change the delegated contract.
Any drawbacks?
As a result of its extreme simplicity, the submission is a bit lacking in framing an actual real-life scenario where this might happen. Improvements in either the storytelling or the smart contract system would have been welcomed.
Why did we chose this submission to be one of the winners?
Its simplicity is really good. The attack could not have been more straight-forward than it was and still relevant in todayâs security landscape of the EVM.
Reiterating the answer to the previous question, this extreme simplicity is also a reason for it to not be the strongest of submissions. Some more context wouldâve been welcomed.
2ď¸âŁ nd Place: Jaime Iglesias
commentary by Austin Williams
In this submission, an EIP-1967 proxy is used for upgradability of an ERC-20 token contract. There are two key roles here: an admin and an owner. There is an _optIn
storage variable that indicates whether or not the owner has consented to an upgrade. The admin should not be able to upgrade unless the owner has set the _optIn
value to true
.
The _optIn
storage variable is stored in storage slot 0x7b191067458f5b5c0c36f2be8ceaf27679e7ea94b6964093ce9e5c7db2aff82a
, which the code comments claim is derived via âthe keccak-256 hash of âeip1967.proxy.optInâ subtracted by 1â. However, the storage location actually points to the location in storage where the token balance of an admin-known address is stored. This means the admin can set _optIn
to true
without the ownerâs consent simply by sending some tokens to the admin-known address â thereby changing the value at the _optIn
storage location from 0
to a truthy value.
In this way, the back door can be unlocked and locked simply by sending tokens to and from some address that is controlled by the admin.
1ď¸âŁ st Place: Robert M C Forster
commentary by Chris Reitwiessner
This submission implements a nicely structured and very simple timelock upgrade: The owner proposes a new implementation of the delegate which will get active one month after it has been proposed.
The only suspicious part about it is that the point in time where the upgrade is performed is stored as month and day instead of a timestamp. The exploit is very well hidden and makes use of special unicode symbols that change the text flow direction. Such symbols are of course invalid in code, but they are accepted in comments.
Since multi-line comments work in the same way when read from left to right as well as from right to left, the submission is able to terminate the comment âfrom right to leftâ and switches into code mode which is then displayed in the âwrongâ order. This causes the day and month variables to be swapped unnoticed and the upgrade is performed much earlier than expected.
You can see (or not, depending on your editor đŹ) the direction change in action by selecting part of line 65 in the submission.
This is a flaw that can be prevented at the level of Solidity by disallowing text direction changes to flow across comments (issue tracker), but I would also call this to the attention of IDE, highlighter and linter developers. I am not sure how a linter would handle such a situation, but if a linter marks superfluous whitespace at the end of a line, it should certainly mark a text direction change flowing out of a comment.
Honorable Mentions
đŚ Chris Whinfreyâs VampireSwap
Props for the witty and detailed storytelling and documentation!
This submission shows the social aspect of trusting âauditedâ upgradable contracts: unless someone verifies every single on-chain executed upgrade (plus the initialization), none of the audits matter. Not even the one from Open Trail of Diligence! ;)
đ Luiz Soares & Boris Breslavâs Superior Proxy
This submissions exploits a Solidity compiler bug, namely the Solidity Dynamic Array Cleanup Bug, which was fixed with Solidity v0.7.3. Using this bug is a great idea in theory, however, the implementation is suspicious and would raise eyebrows.
See you at the next Underhanded Solidity Contest!
We would like to thank ConsenSys Diligence, Solidified, OpenZeppelin, and the Ethereum Foundation for contributing prizes and our judges Alex Beregszaszi, Austin Williams, Christian Reitwiessner, Gonçalo Så, and Stefan Beyer for their assessments and support!
Would you like to propose a topic for the next Underhanded Solidity Contest, provide feedback, sponsor a prize, or help with judging the next time? Then feel free to reach out to us at sol_underhanded@ethereum.org!