Create a gist now

Instantly share code, notes, and snippets.

What would you like to do?

Section 1 - Table of Contents

Section 2 - Introduction

From March 24th through March 31st of 2017, Piper Merriam conducted an audit of the Etheroll smart contracts. The findings of the audit are presented in this document.

This audit was performed under a contracted hourly rate with no other compensation.

Disclaimer that Piper Merriam currently holds 10,100 etheroll tokens purchased through the crowdsale.

2.1 Authenticity

This document should have an attached cryptographic signature to ensure it has not been tampered with. The signature can be verified using the public key from Piper Merriam's the keybase.io.

Piper Merriam on Keybase.io

2.2 Audit Goals and Focus

2.2.1 Sound Architecture

This audit includes both objective findings from the contract code as well as subjective assessments of the overall architecture and design choices. Given the subjective nature of certain findings it will be up to the Etheroll development team to determine the appropriate response to each issue.

2.2.2 Smart Contract Best Practices

This audit will evaluate whether the codebase follows the current established best practices for smart contract development.

2.2.3 Code Correctness

This audit will evaluate whether the code does what it is intended to do.

2.2.4 Code Quality

This audit will evaluate whether the code has been written in a way that ensures readability and maintainability.

2.2.5 Security

This audit will look for any exploitable security vulnerabilities, or other potential threats to the integrity of the Etheroll system. Primary focuses are:

  • Ensure investor funds in the bankroll are secure.
  • Ensure that user funds are secure.
  • Ensure that the game rules are accurately represented in the code.

2.3 About Etheroll

The primary Etheroll website can be found at etheroll.com. That website has this to say about Etheroll.

Etheroll is an Ethereum smart contract for placing bets on our provably-fair dice game using Ether with no deposits or sign-ups. Each dice roll is provably random and cryptographically secure thanks to the nature of the Ethereum blockchain.

2.4 Terminology

This audit uses the following terminology.

2.4.1 Coverage

Measurement of the testing code coverage. This measurement was done via inspection of the code.

2.4.1.1 untested

No tests.

2.4.1.2 low

The tests do not cover some set of non-trivial functionality.

2.4.1.3 good

The tests cover all major functionality.

2.4.1.4 excellent

The tests cover all code paths.

2.4.2 Severity

Measurement of magnitude of an issue.

2.4.2.1 minor

Minor issues are generally subjective in nature, or potentially deal with topics like "best practices" or "readability". Minor issues in general will not indicate an actual problem or bug in code.

The maintainers should use their own judgement as to whether addressing these issues improves the codebase.

2.4.2.2 medium

Medium issues are generally objective in nature. Most medium level issues will not represent an actively exploitable bugs or security problem, but rather an issue that is likely to lead to a future error or security issue.

In most cases a medium issue should be addressed unless there is a clear reason not to.

2.4.2.3 major

Major issues will be things like bugs or security vulnerabilities. These issues may not be directly exploitable such as requiring a specific condition to arise in order to be exploited.

Left unaddressed these issues are highly likely to cause problems with the operation of the contract or lead to a situation which allows the system to be exploited in some way.

2.4.2.4 critical

Critical issues are directly exploitable bugs or security vulnerabilities.

Left unaddressed these issues are highly likely or guaranteed to cause major problems or potentially a full failure in the operations of the contract.

Section 3 - Overview

3.1 Source Code

The smart contract source code was made available in a private repository on github.

The code being evaluated was found under the commit hash 5a1ab3b3c966f48c10f37c0047966aac191eb6dd

This audit covers the following single Solidity source file.

  • ./contracts/Etheroll.sol

The sha256 of that contract source is as follows.

$ shasum -a 256 ./contracts/Etheroll.sol
6792724057484bda1ff7fbf86f98ab8654a7f43aacd157712e0d8ea5d1ff2a48  ./contracts/Etheroll.sol

3.2 Contracts

Thes source file ./contracts/Etheroll.sol contains a number of abstract base contracts which are combined into the single Etheroll contract.

Section 4 - General Findings

This section contains general findings which are not necessarily related to a specific part of the contract code.

4.1 General Thoughts

The changes to the contract code between the previous audit and this one are largely superficial.

  • Changed betId type from int to bytes32
  • Bet validity now checks against profit rather than bet amount.
  • Rename maxBetDivisor to maxProfitDivisor
  • Rename maxBet to maxProfit
  • Rename maxBetAsPercentOfHouse to maxProfitAsPercentOfHouse
  • Add priviledged function for updating oraclize cost.
  • Track total Wei that has been won.
  • Addition of LogOwnerTransfer event.
  • Addition of LogRefund event.
  • Modify how random roll result is computed.
  • Addition of 1 Wei consolation prize.

Examination of these changes did not yield any new exploitable issues.

4.2 Minor Issues

4.2.1 The 1 wei reward for losing bets is likely unnecessary.

When a player loses their bet, they are sent 1 wei as a sort of consolation prize. At the current price of ether, this is $0.00000000000000005. The transaction cost for sending this amount is multiple orders of magnitude than the amount itself.

In cases where the sending fails, this amount is set aside for the user to withdraw. Withdrawal of this amount will cost roughly 14 orders of magnitude more than the amount itself meaning these rewards are likely to never be withdrawn.

4.2.2 Oraclize can control dice rolls

The Oraclize service is capable of controlling the result of dice rolls. The process to do this is as follows.

  • Make random.org API call
  • Generate TLS proof
  • Generate IPFS URI for proof
  • Precompute sha3(.....) % 100 + 1.
  • If the result is the desired roll, submit result to the __callback function.
  • Otherwise return to step 1.

While the Etheroll contract cannot prevent this action, it does take steps to ensure it is dectectable. Each API result returned by Random.org contains a serial number which increases by 1 for each API request.

The Etheroll contract logs this value making this type of exploit detectable.

No change recommended for this issue.

4.3 Medium Issues

None

4.4 Major Issues

None

4.5 Critical Issues

None

4.6 Test Coverage Analysis

The test coverage for this codebase appears to not have been changed since the previous audit.

The tests cover a small number of utility functions but do not appear to cover any of the core functionality.

Section 5 - Detailed Findings

The following is an exhaustive list of all issues found during in this audit.

5.1 Minor Issues

5.1.1 Winnings formula will underflow for very low bet values

The following formula is used to compute a player's winnings.

((((msg.value * (100-(safeSub(rollUnder,1)))) / (safeSub(rollUnder,1))+msg.value))*houseEdge/houseEdgeDivisor)-msg.value;

The formula is constrained as follows.

  • The rollUnder value is constrained between 2 and 99 (inclusive).
  • The houseEdge is set to 990
  • The houseEdgeDivisor is set to 1000
  • The msg.value is required to be 100000000000000000 (0.1 ether) or greater.

This formula will overflow for very small msg.value values. This is likely not a concern since the minimum bet is likely to always be sufficiently high. In addition, in the event that it does underflow, it will exceed the maxProfit value checks.

Recommend just being aware that this is possible in case future changes to this formula alter the underflow conditions.

5.1.2 The LogResult events are logged using magic numbers.

The places that log the LogResult event use integer values for the status field.

LogResult(...., 2, ...);                   

There is a comment accompanying the declaration of the LogResult event which denotes what each of these integer values represent.

/* Status: 0=lose, 1=win, 2=win + failed send, 3=refund, 4=refund + failed send*/
event LogResult(bytes32 indexed BetID, address indexed PlayerAddress, uint indexed PlayerNumber, uint DiceResult, uint Value, int Status, bytes Proof);   

While it is good that there is a reference, it makes the code more difficult to read.

Recommend either using an Enum to represent these status values or using constant variables for each.

enum Status {
    Lose,
    Win,
    WinWithSendFail,
    Refund,
    RefundWithSendFail
}

// or

uint constant LOSE = 0;
uint constant WIN = 0;
uint constant WIN_WITH_SEND_FAIL = 0;
uint constant REFUND = 0;
uint constant REFUND_WITH_SEND_FAIL = 0;

5.2 Medium Issues

5.2.1 The formula for calculating winnings is duplicated in code.

The formula for calculating winnings is duplicated in two locations.

  • Line 302
  • Line 450

Any changes to the formula must be made in two places. This type of duplication can lead to errors, such as the case where only one of these formulas is modified, resulting in inconsistent business logic.

Recommend creating a function which does this computation.

5.3 Major Issues

None

5.4 Critical Issues

None

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
# Section 1 - Table of Contents<a id="heading-0"/>
* 1 - [Table of Contents](#heading-0)
* 2 - [Introduction](#heading-2)
* 2.1 - [Authenticity](#heading-2.1)
* 2.2 - [Audit Goals and Focus](#heading-2.2)
* 2.2.1 - [Sound Architecture](#heading-2.2.1)
* 2.2.2 - [Smart Contract Best Practices](#heading-2.2.2)
* 2.2.3 - [Code Correctness](#heading-2.2.3)
* 2.2.4 - [Code Quality](#heading-2.2.4)
* 2.2.5 - [Security](#heading-2.2.5)
* 2.3 - [About Etheroll](#heading-2.3)
* 2.4 - [Terminology](#heading-2.4)
* 2.4.1 - [Coverage](#heading-2.4.1)
* 2.4.1.1 - [**untested**](#heading-2.4.1.1)
* 2.4.1.2 - [**low**](#heading-2.4.1.2)
* 2.4.1.3 - [**good**](#heading-2.4.1.3)
* 2.4.1.4 - [**excellent**](#heading-2.4.1.4)
* 2.4.2 - [Severity](#heading-2.4.2)
* 2.4.2.1 - [**minor**](#heading-2.4.2.1)
* 2.4.2.2 - [**medium**](#heading-2.4.2.2)
* 2.4.2.3 - [**major**](#heading-2.4.2.3)
* 2.4.2.4 - [**critical**](#heading-2.4.2.4)
* 3 - [Overview](#heading-3)
* 3.1 - [Source Code](#heading-3.1)
* 3.2 - [Contracts](#heading-3.2)
* 4 - [General Findings](#heading-4)
* 4.1 - [General Thoughts](#heading-4.1)
* 4.2 - [Minor Issues](#heading-4.2)
* 4.2.1 - [The 1 wei reward for losing bets is likely unnecessary.](#heading-4.2.1)
* 4.2.2 - [Oraclize can control dice rolls](#heading-4.2.2)
* 4.3 - [Medium Issues](#heading-4.3)
* 4.4 - [Major Issues](#heading-4.4)
* 4.5 - [Critical Issues](#heading-4.5)
* 4.6 - [Test Coverage Analysis](#heading-4.6)
* 5 - [Detailed Findings](#heading-5)
* 5.1 - [Minor Issues](#heading-5.1)
* 5.1.1 - [Winnings formula will underflow for very low bet values](#heading-5.1.1)
* 5.1.2 - [The `LogResult` events are logged using *magic* numbers.](#heading-5.1.2)
* 5.2 - [Medium Issues](#heading-5.2)
* 5.2.1 - [The formula for calculating winnings is duplicated in code.](#heading-5.2.1)
* 5.3 - [Major Issues](#heading-5.3)
* 5.4 - [Critical Issues](#heading-5.4)
# <a id="heading-2"/> Section 2 - Introduction
From March 24th through March 31st of 2017, Piper Merriam conducted an audit of the
Etheroll smart contracts. The findings of the audit are presented in this
document.
This audit was performed under a contracted hourly rate with no other compensation.
Disclaimer that Piper Merriam currently holds 10,100 etheroll tokens purchased
through the crowdsale.
## <a id="heading-2.1"/> 2.1 Authenticity
This document should have an attached cryptographic signature to ensure it has
not been tampered with. The signature can be verified using the public key
from Piper Merriam's the keybase.io.
[Piper Merriam on Keybase.io](https://keybase.io/pipermerriam)
## <a id="heading-2.2"/> 2.2 Audit Goals and Focus
### <a id="heading-2.2.1"/> 2.2.1 Sound Architecture
This audit includes both objective findings from the contract code as well as
subjective assessments of the overall architecture and design choices. Given
the subjective nature of certain findings it will be up to the Etheroll
development team to determine the appropriate response to each issue.
### <a id="heading-2.2.2"/> 2.2.2 Smart Contract Best Practices
This audit will evaluate whether the codebase follows the current established
best practices for smart contract development.
### <a id="heading-2.2.3"/> 2.2.3 Code Correctness
This audit will evaluate whether the code does what it is intended to do.
### <a id="heading-2.2.4"/> 2.2.4 Code Quality
This audit will evaluate whether the code has been written in a way that
ensures readability and maintainability.
### <a id="heading-2.2.5"/> 2.2.5 Security
This audit will look for any exploitable security vulnerabilities, or other
potential threats to the integrity of the Etheroll system. Primary focuses
are:
* Ensure investor funds in the *bankroll* are secure.
* Ensure that user funds are secure.
* Ensure that the game rules are accurately represented in the code.
## <a id="heading-2.3"/> 2.3 About Etheroll
The primary Etheroll website can be found at
[etheroll.com](http://etheroll.com). That website has this to say about
Etheroll.
> Etheroll is an Ethereum smart contract for placing bets on our provably-fair dice game using Ether with no deposits or sign-ups. Each dice roll is provably random and cryptographically secure thanks to the nature of the Ethereum blockchain.
## <a id="heading-2.4"/> 2.4 Terminology
This audit uses the following terminology.
### <a id="heading-2.4.1"/> 2.4.1 Coverage
Measurement of the testing code coverage. This measurement was done via
inspection of the code.
#### <a id="heading-2.4.1.1"/> 2.4.1.1 **untested**
No tests.
#### <a id="heading-2.4.1.2"/> 2.4.1.2 **low**
The tests do not cover some set of non-trivial functionality.
#### <a id="heading-2.4.1.3"/> 2.4.1.3 **good**
The tests cover all major functionality.
#### <a id="heading-2.4.1.4"/> 2.4.1.4 **excellent**
The tests cover all code paths.
### <a id="heading-2.4.2"/> 2.4.2 Severity
Measurement of magnitude of an issue.
#### <a id="heading-2.4.2.1"/> 2.4.2.1 **minor**
Minor issues are generally subjective in nature, or potentially deal with
topics like *"best practices"* or *"readability"*. Minor issues in general
will not indicate an actual problem or bug in code.
The maintainers should use their own judgement as to whether addressing these
issues improves the codebase.
#### <a id="heading-2.4.2.2"/> 2.4.2.2 **medium**
Medium issues are generally objective in nature. Most medium level issues will
not represent an actively exploitable bugs or security problem, but rather an
issue that is *likely* to lead to a future error or security issue.
In most cases a medium issue should be addressed unless there is a clear reason
not to.
#### <a id="heading-2.4.2.3"/> 2.4.2.3 **major**
Major issues will be things like bugs or security vulnerabilities. These
issues may not be directly exploitable such as requiring a specific condition
to arise in order to be exploited.
Left unaddressed these issues are highly likely to cause problems with the
operation of the contract or lead to a situation which allows the system to be
exploited in some way.
#### <a id="heading-2.4.2.4"/> 2.4.2.4 **critical**
Critical issues are directly exploitable bugs or security vulnerabilities.
Left unaddressed these issues are highly likely or guaranteed to cause major
problems or potentially a full failure in the operations of the contract.
# <a id="heading-3"/> Section 3 - Overview
## <a id="heading-3.1"/> 3.1 Source Code
The smart contract source code was made available in a private repository on
github.
The code being evaluated was found under the commit hash
`5a1ab3b3c966f48c10f37c0047966aac191eb6dd`
This audit covers the following single Solidity source file.
- - `./contracts/Etheroll.sol`
The sha256 of that contract source is as follows.
```bash
$ shasum -a 256 ./contracts/Etheroll.sol
6792724057484bda1ff7fbf86f98ab8654a7f43aacd157712e0d8ea5d1ff2a48 ./contracts/Etheroll.sol
```
## <a id="heading-3.2"/> 3.2 Contracts
Thes source file `./contracts/Etheroll.sol` contains a number of abstract base
contracts which are combined into the single `Etheroll` contract.
# <a id="heading-4"/> Section 4 - General Findings
This section contains general findings which are not necessarily related to a
specific part of the contract code.
## <a id="heading-4.1"/> 4.1 General Thoughts
The changes to the contract code between the previous audit and this one are
largely superficial.
* Changed `betId` type from `int` to `bytes32`
* Bet validity now checks against profit rather than bet amount.
* Rename `maxBetDivisor` to `maxProfitDivisor`
* Rename `maxBet` to `maxProfit`
* Rename `maxBetAsPercentOfHouse` to `maxProfitAsPercentOfHouse`
* Add priviledged function for updating oraclize cost.
* Track total Wei that has been won.
* Addition of `LogOwnerTransfer` event.
* Addition of `LogRefund` event.
* Modify how random roll result is computed.
* Addition of 1 Wei consolation prize.
Examination of these changes did not yield any new exploitable issues.
## <a id="heading-4.2"/> 4.2 Minor Issues
### <a id="heading-4.2.1"/> 4.2.1 The 1 wei reward for losing bets is likely unnecessary.
When a player loses their bet, they are sent 1 wei as a sort of consolation
prize. At the current price of ether, this is `$0.00000000000000005`. The
transaction cost for sending this amount is multiple orders of magnitude than
the amount itself.
In cases where the sending fails, this amount is set aside for the user to
withdraw. Withdrawal of this amount will cost roughly 14 orders of magnitude
more than the amount itself meaning these rewards are likely to never be
withdrawn.
### <a id="heading-4.2.2"/> 4.2.2 Oraclize can control dice rolls
The Oraclize service is capable of controlling the result of dice rolls. The
process to do this is as follows.
* Make random.org API call
* Generate TLS proof
* Generate IPFS URI for proof
* Precompute `sha3(.....) % 100 + 1`.
* *If* the result is the desired roll, submit result to the `__callback` function.
* *Otherwise* return to step 1.
While the Etheroll contract cannot prevent this action, it does take steps to
ensure it is dectectable. Each API result returned by Random.org contains a
serial number which increases by 1 for each API request.
The Etheroll contract logs this value making this type of exploit detectable.
No change recommended for this issue.
## <a id="heading-4.3"/> 4.3 Medium Issues
None
## <a id="heading-4.4"/> 4.4 Major Issues
None
## <a id="heading-4.5"/> 4.5 Critical Issues
None
## <a id="heading-4.6"/> 4.6 Test Coverage Analysis
The test coverage for this codebase appears to not have been changed since the
previous audit.
The tests cover a small number of utility functions but do not appear to cover
any of the core functionality.
# <a id="heading-5"/> Section 5 - Detailed Findings
The following is an exhaustive list of all issues found during in this audit.
## <a id="heading-5.1"/> 5.1 Minor Issues
### <a id="heading-5.1.1"/> 5.1.1 Winnings formula will underflow for very low bet values
The following formula is used to compute a player's winnings.
```javascript
((((msg.value * (100-(safeSub(rollUnder,1)))) / (safeSub(rollUnder,1))+msg.value))*houseEdge/houseEdgeDivisor)-msg.value;
```
The formula is constrained as follows.
* The `rollUnder` value is constrained between 2 and 99 (inclusive).
* The `houseEdge` is set to `990`
* The `houseEdgeDivisor` is set to `1000`
* The `msg.value` is required to be `100000000000000000` (0.1 ether) or greater.
This formula will overflow for very small `msg.value` values. This is likely
not a concern since the minimum bet is likely to always be sufficiently high.
In addition, in the event that it does underflow, it will exceed the
`maxProfit` value checks.
Recommend just being aware that this is possible in case future changes to this
formula alter the underflow conditions.
### <a id="heading-5.1.2"/> 5.1.2 The `LogResult` events are logged using *magic* numbers.
The places that log the `LogResult` event use integer values for the `status`
field.
```javascript
LogResult(...., 2, ...);
```
There is a comment accompanying the declaration of the `LogResult` event which
denotes what each of these integer values represent.
```javascript
/* Status: 0=lose, 1=win, 2=win + failed send, 3=refund, 4=refund + failed send*/
event LogResult(bytes32 indexed BetID, address indexed PlayerAddress, uint indexed PlayerNumber, uint DiceResult, uint Value, int Status, bytes Proof);
```
While it is good that there is a reference, it makes the code more difficult to
read.
Recommend either using an Enum to represent these status values or using
`constant` variables for each.
```
enum Status {
Lose,
Win,
WinWithSendFail,
Refund,
RefundWithSendFail
}
// or
uint constant LOSE = 0;
uint constant WIN = 0;
uint constant WIN_WITH_SEND_FAIL = 0;
uint constant REFUND = 0;
uint constant REFUND_WITH_SEND_FAIL = 0;
```
## <a id="heading-5.2"/> 5.2 Medium Issues
### <a id="heading-5.2.1"/> 5.2.1 The formula for calculating winnings is duplicated in code.
The formula for calculating winnings is duplicated in two locations.
* Line 302
* Line 450
Any changes to the formula must be made in two places. This type of duplication can lead to errors, such as the case where only one of these formulas is modified, resulting in inconsistent business logic.
Recommend creating a function which does this computation.
## <a id="heading-5.3"/> 5.3 Major Issues
None
## <a id="heading-5.4"/> 5.4 Critical Issues
None
-----BEGIN PGP SIGNATURE-----
Version: Keybase OpenPGP v2.0.68
Comment: https://keybase.io/crypto
wsFcBAABCgAGBQJY7k3HAAoJEI0iZdXOvoIvhAsP/i3GEZjUCXetfhnFPYCQor78
/QX9NBaBRcYuJaIqm2gq5815ZRJMq7C1mF1GqIUWBaixjIfPQReH2rSTHBlHcvFy
Fm6z/C4BQf/aIG7bvOLP96jJJwUEEvmLJ0hoWuFhpsmmDtDyqa0k53Vb+M+Bmfi4
CqRXoPF42sOohIIuxCqnItlKHdju6UI/iCgdkI9XemXK8emj2ZsqIbGkl9annoyo
/9Hi9AwsH2JPhZ00rhr49rh6prnBK/RD7Drw+IBjSkzOH6+xsp+CuA6tOX4ZeP5/
xg0w7KElWGcfFdPqKzKmw3xcXX5d48pou28f0KGLV6SNkiElaRChW7dgOJuv4RDz
/ONArjSoEvLNqcP/E35PEbiAjRI48oX1Q5HS7j45b2bm7uDGszUgEQkm4DZ2BMtx
1KPI6/k8cAQYuPKO28+i9KtOUuef6YHSWULs+ubaeglQ3h1qrQya7GDEwgEZ+KX5
M7TZrv/ezPQ3RdxfhujXexpHOn0h2Lop4pLIeNarmq5Pr1uDSyfvSKqW7knXz5bJ
m2nZfiPlOFZKvjj+9kwonFSmexXndU3jYFjSX28NcCGR6y1lvFy55jOsanlojvu4
86I8UsPMcs7YK+VtGNbNxicXxom13dYhIhJ74IR/IP+AOBV4MZWvs5Jx6IbxKd+R
EtbISg+4Sf/lJ5oPGMwZ
=Nb6s
-----END PGP SIGNATURE-----
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment