This is the report from a security audit performed on dice-game by by alexo18. The audit focused primarily on the security of funds and fault tolerance of the dice-game contract. The main intention of this contract is to serve as decentralized casino game.
In total, 4 issues were reported including:
-
1 medium severity issues.
-
4 low severity issues.
No critical security issues were found.
Being not initialized, this zero initialized variable can affect contract code behave in unintended way.
https://github.com/craz3049/etc-dice-game/blob/master/dice1.sol#L5
Initialize the variable from the "CoinFlip()" constructor as follows:
..{
owner = msg.sender;
}
All the functions of dice1.sol contract have no visibility specifier. It is strongly recommended to adhere to the same coding standard when developing components of blockchain systems.
https://github.com/craz3049/etc-dice-game/blob/master/dice1.sol#L10
https://github.com/craz3049/etc-dice-game/blob/master/dice1.sol#L22
https://github.com/craz3049/etc-dice-game/blob/master/dice1.sol#L43
Explicitly implement appropriate visibility specifier for dice1.sol functions as follows:
function CoinFlip() public payable {
...
}
In case of "winning" this function checks if accumulated contract balance is enough to pay winning prize, but there is mathematical mistake : the function checks only for availability of the premium , but should check for msg.value + premium instead. (Value of "this.balance" in payable methods is increased by "msg.value before" the body executes.)
https://github.com/craz3049/etc-dice-game/blob/master/dice1.sol#L24
use
"if(this.balance < (msg.value * (100 + payPercentaje)/100)) {"
instead of
"if(this.balance < ((msg.value*payPercentaje)/100)) {"
-
The construction of the "onlyOwner" modifier is somewhat cumbersome and not standard , much better to use here conventional "require(msg.sender == owner);" construction.
-
In order to make events stand out with regards to regular function calls, "emit" EventName() as opposed to just EventName() should be used to "call" events.
-
Given the presence of "kill()" function is a good practice to provide an explicit state variable and modifier to indicate contract status for "FlipCoin()" function caller, otherwise funds sended to the "FlipCoin()" function will be lost after "kill()" execution.
-
The payable constructor function is executed once , so point for consideration here is to introduce an explicit "deposit" function as follows:
function deposit(uint256 amount) payable public onlyOwner { }
The contract is not ready to use at the time of the audit due to presence of some non-critical bugs. The last fixation was made on May 16, 2018. the last commit was made at May 16, 2018: https://github.com/craz3049/etc-dice-game/blob/master/dice1.sol
Bug Bounties were not conducted. Any further changes to the contracts will leave them in unaudited state.
No critical vulnerabilities were detected. The reported issues can not directly hurt the dice-game smart-contract. The dice-game smart-contract can satisfy the main goal and could be used for dice-game contract after completion aforementioned bugs list.