Don’t “Go Freaking Do It“ - Smart Contract Analysis

Just saw a post on Hacker News about a great idea for an Ethereum smart contract called “Go Freaking Do It” and decided to take a look at how it works. This post describes some of the bugs we found.

Intro

My friend Nico and I are both interested in information/cyber security, and have recently taken notice to the attack surfaces available in Smart Contracts. Currently, we’re looking mainly at Solidity as well as EVM bytecode, trying to automate the process of finding vulnerabilities as well as doing some manual code audits.

The code for the contract is on Etherscan and is available for auditing directly there. If you’re unfamiliar with Solidity, take a look at this cheat sheet. Props to Karolis Ramanauskas who is the author of the contract, and executed such a neat idea.

What is “Go Freaking Do It”

The concept is a simple: you deposit $$$ into the contract together with the goal you’d like to accomplish, as well as an optional “supervisor” contact who will verify if you’ve accomplished the goal or not by a given date. If they say you’ve done it, then you get your $$$ back. If not, “Go Freaking Do It” keeps it and the owner earns some money for their effort of building this.

Low Severity Problems Found

Data Loss. Not checking if the goals already contain given hash. Could result in lost goals (and $$$) if a goal with identical sender/description/value/deadline are submitted.

function setGoal() {(...snip for brevity...)

bytes32 hash = keccak256(msg.sender, \_description, msg.value, \_deadline);

Goal memory goal = Goal(...snip for brevity...);

goals\[hash\] = goal;

(...snip for brevity...)  

}

Solution: Either reject duplicates or add a nonce to the hash.

One super user for all tasks. This contract has two classes of users, “owner” and everyone else. The problem with this is that the contract has a few house keeping functions which are presumably run on an external system somewhere (such as setEmailSent()), and this system will then have access to credentials which can do absolutely anything to the contract. (syphon the money, transfer ownership, destroying it.).

Solution: It would be better to tier the permissions and have an additional account, “accountant” or similar.

Data Leak. Because all goals are public, then you can see the e-mail addresses, goals, and amounts entered by all users.

Goal[] public activeGoals;

Solution: Don’t mark as public.

High Severity Problems Found

No permissioning for setting a goal as failed. The “onlyOwner” guard is missing for setGoalFailed. This means that anyone can “fail” the goals of every user in the contract at any time, essentially donating all their money to the contract owner.

function setGoalFailed(uint _index, bytes32 _hash) {assert(goals[_hash].amount > 0);// assert(goals[_hash].emailSent == true);

goals\[\_hash\].completed = false;  
activeGoals\[\_index\].completed = false;

owner.transfer(goals\[\_hash\].amount); // send ether to contract owner

setGoalFailedEvent(\_hash, false);  

}

Solution: Add the onlyOwner guard to the function declaration.

No Versioning or upgrade possibilities. Because all of the state is stored in the contract itself, then upgrading is impossible.

Solution: Having separate business and data logic, alternatively, making use of delegatecall could help here. See here and here for great articles about upgradable contracts.

Who we are

Two FinTech CTOs for hire, based in London, UK.

More about Jesper | More about Nico.

Notes

Shortly before publishing this post, user “mpeg” on HN discovered the same setGoalFailed() bug.

Update

Awesome news! 😎 The author pointed out that he’s working on a V2 and will address the outstanding issues.