/etheroll-final-report.md Secret
Last active
September 6, 2018 15:39
Revisions
-
pipermerriam revised this gist
Apr 12, 2017 . 2 changed files with 25 additions and 17 deletions.There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -94,8 +94,12 @@ 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 This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -97,8 +97,12 @@ 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 @@ -396,17 +400,17 @@ None 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----- -
Apr 11, 2017 .There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -0,0 +1,391 @@ # 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 ENS system as well as user funds held by ENS contracts. ## <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 This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode charactersOriginal file line number Diff line number Diff line change @@ -0,0 +1,412 @@ -----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 ENS system as well as user funds held by ENS contracts. ## <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 wsFcBAABCgAGBQJY7T4gAAoJEI0iZdXOvoIvXKsP/A5czL/mH4n3HcX0KH0OTadt 8ZwowtkLOwxSTho2oEgS2mqoucfbYijtJUrvpFz/8uOO8QJ5B94MXkE9aDht+TLm QzAx+zT7Ol+6aN8jdx8rQ76vVRFgcdumT/VWDs+48lf5MQm7pNPmowznW+/TkId4 7hVsUn16IU7+RUt65l4KfcKvc5VKQwn6j5Xmv882kvvgwih1uOCEaEs/9iwunH64 9AQ4opHU/iic2ql6tHCb5NkxIkx6sigWIjiLYXP+xWg2eoqCvWDiUsDDLU4WQ/39 /zzQQY7h5foDq/MapxO1Qeywt1MT3GZ3q90417chV1X0shlf3qvy3ryTVAaSIdi0 OBtLBuyCVepS8Z2+M6Qki/DFkL2xQik4DOxwG8ywUDwg3kvkZWtNUUezURYZ8aQu GWmLjmNvvQir9/DZK4mZUQr9hPjUeyDVY802H+T9OInPR4o8WMymSdiptsVEtr9E nNPnAYSTk3Pq351F9qrK78+ONH+kR7ZMrwYSSGbKrh/v/LE8VA4jv4Q/xR/0/G1Z hHyIVt+cXMf/rA3rz65PbZN0+JavxMdmuWg1MIBa63PTAVUgu9PCzxM4mraMZxFR co2s7e8K8LolfP5p4G6gR5qdTdJyjy7x7Ze8arep0DEsOKXq1ooHgVmtltmeAVcs ++E/KZ9fx+aAZEpD7zIh =UTbV -----END PGP SIGNATURE-----