Dynamic Array Stack Corruption

This contract appears to be a game offering 1/16 odds at the balance of the contract. However, when running the code in a debugger it appears as if the 'secretNumber' variable is being overwritten before it is used.

pragma solidity ^0.4.19;

contract CryptoRoulette {

    uint256 private secretNumber;
    uint256 public lastPlayed;
    uint256 public betPrice = 0.1 ether;
    address public ownerAddr;

    struct Game {
        address player;
        uint256 number;
    }
    Game[] public gamesPlayed;

    function CryptoRoulette() public {
        ownerAddr = msg.sender;
        shuffle();
    }

    function shuffle() internal {
        // initialize secretNumber with a value between 0 and 15
        secretNumber = uint8(sha3(now, block.blockhash(block.number-1))) % 16;
    }

    function play(uint256 number) payable public {
        require(msg.value >= betPrice && number < 16);

        Game game;
        game.player = msg.sender;
        game.number = number;
        gamesPlayed.push(game);

        if (number == secretNumber) {
            // win!
            msg.sender.transfer(this.balance);
        }

        shuffle();
        lastPlayed = now;
    }

    function kill() public {
        if (msg.sender == ownerAddr && now > lastPlayed + 1 days) {
            suicide(msg.sender);
        }
    }

    function() public payable { }
}

The way secretNumber is updated, it should always be less than 16 secretNumber = uint8(sha3(now, block.blockhash(block.number-1))) % 16;

This debugger output shows that during execution of if (number == secretNumber) { the value of secretNumber has been updated to, oddly enough, the callers address (msg.sender).

`
(243) PUSH1 0x00
  000000000000000000000000000000000000000000000000000000006898f82b
  0000000000000000000000000000000000000000000000000000000000000143
  0000000000000000000000000000000000000000000000000000000000000003
  0000000000000000000000000000000000000000000000000000000000000000 (top)

40:         if (number == secretNumber) {
                          ^^^^^^^^^^^^

debug(develop:0x98cacf83...)> i

CryptoRoulette.sol | 0xbd2c938b9f6bfc1a66368d08cb44dc3eb2ae27be:

40:         if (number == secretNumber) {
                ^^^^^^

debug(develop:0x98cacf83...)> p

CryptoRoulette.sol | 0xbd2c938b9f6bfc1a66368d08cb44dc3eb2ae27be:

(245) DUP3
  000000000000000000000000000000000000000000000000000000006898f82b
  0000000000000000000000000000000000000000000000000000000000000143
  0000000000000000000000000000000000000000000000000000000000000003
  0000000000000000000000000000000000000000000000000000000000000000
  000000000000000000000000627306090abab3a6e1400e9345bc60c78a8bef57 (top)

40:         if (number == secretNumber) {
                ^^^^^^

My guess is that the storage access before the condition is causing the stack to be corrupted somehow.

Is this a known vulnerability? Can someone please explain what is going on?

Solution

This is a common issue when attempting to create a local reference without specifying the correct storage location.

From the Solidity docs:

There are defaults for the storage location depending on which type of variable it concerns:

state variables are always in storage

function arguments are in memory by default

local variables of struct, array or mapping type reference storage by default

local variables of value type (i.e. neither array, nor struct nor mapping) are stored in the stack

The bolded comment indicates that the line Game game; defaults to storage. If you don't initialize a storage variable, it will point to storage slot 0 by default. The end result is when you make a change to game (with game.player = msg.sender;), it will write the value out to the first slot, which will be whatever the first variable is in your contract (in this case, secretNumber).

The correct way to write this is to use Game memory game;. The compiler will give you a warning if you omit the memory keyword for this exact reason.