Trail of Bits has been maintaining a great collection of poorly written and/or exploitable Solidity contracts in a Github repo: https://github.com/trailofbits/not-so-smart-contracts
I highly recommend reading through all of them, but let’s take an easy one apart together to see the kind of things they’re looking at.
Variable shadowing is one of those frustrating errors that can pop up in a lot of programming languages. The compiler won’t throw an error, but your code just doesn’t work as expected for some reason. The truth is, although many compilers won’t throw an error, it probably gave you a nice yellow warning that you completely ignored.
It’s a pretty straightforward and easy mistake to make. Here is the example Trail of Bits gives:
The crux of the problem is that there are two variables named “owner”, and both of them are in different scopes. In contract C, owner is being set to the msg.sender, but that version of “owner” is only in contract C. The “owner” that’s in contract “Suicidal” is never set. Therefore, if “suicide()” is ever called, it’s not going to get past that “require(owner == msg.sender)” line since that “owner” was never set.
It’s not easy to totally avoid this type of thing, but there are several things you can do to avoid it as much as possible:
Be aware of the common standard variable names
In Solidity, words like “owner”, “owned”, “msg”, “token” aren’t necessarily reserved keywords, but they are fairly well established terms that are used in widely used utilities and libraries. Don’t use those common terms.
Have descriptive variable names
We’ve talked about this one before. Naming is important in Solidity contracts. Other people are probably going to see your code. Make the variable names as descriptive as possible. Instead of “uint i” name the variable “uint lengthOfPlayersMapping”. You get the idea.
Don’t just copy and paste, look at the helper contracts
In a real world scenario, the mistake above probably happened because the coder grabbed the Suicidal contract from some other source and imported it or copy/pasted it into their new project. Doing things like that for common tasks generally saves a lot of time. Use some of that saved time to actually read over the contract you’re importing once or twice. It could save you a huge headache later on. Maybe read it over three times if the contract or library you’re importing deals with contract creation or destruction.
Variable shadowing is one of the more straightforward contracts Trail of Bits has put together. The rest get progressively more complex and everyone should give them a good read to see what kind of exploits are possible in this new world of Ethereum development.