Omega spinners can become desynchronized

[BinaryQuasar]

Description

The contract stores a mapping of deed identifiers to Spinner structs, as well as an array of omega Spinner structs.

https://github.com/nickjm/cryptospinners-bounty/blob/f8453485fc3ed8c021c2513c5e95a16ea69acb9c/contracts/CryptoSpinnersBase.sol#L54

https://github.com/nickjm/cryptospinners-bounty/blob/f8453485fc3ed8c021c2513c5e95a16ea69acb9c/contracts/CryptoSpinnersBase.sol#L59

Omega spinners are added here:

https://github.com/nickjm/cryptospinners-bounty/blob/f8453485fc3ed8c021c2513c5e95a16ea69acb9c/contracts/CryptoSpinnersBase.sol#L181

The intention is for the array to point to the same Spinner structs as the mapping; e.g. if any spinner that is updated in the mapping (energy change), this should also be reflected in the omega array. However, this does not happen.

Severity

Medium (high likelihood, low impact)

Scenario

A spinner's energy can be updated. If this change is made in the mapping, the change will not be reflected in the array.

Minimal working example:

pragma solidity ^0.4.18;

contract Test {
    struct Struct {
        uint256 value;
    }

    mapping (uint256 => Struct) structMapping; 
    Struct[] structArray;

    function Test() public {
        structMapping[0] = Struct(5);
        structArray.push(structMapping[0]);
    }

    function getValues()
        external
        returns(
            uint256 fromMapping,
            uint256 fromArray,
            uint256 fromMappingAfterChange,
            uint256 fromArrayAfterChange
        )
    {
        fromMapping = structMapping[0].value;
        fromArray = structArray[0].value;

        structMapping[0].value = 10;

        fromMappingAfterChange = structMapping[0].value;
        fromArrayAfterChange = structArray[0].value;
    }
}

Calling getValues() returns (5, 5, 10, 5).

Impact

Currently this has low impact, as omegaSpinners is only used to look up the id value of the spinner.

However, future code might accidentally refer to omegaSpinners energy values, which would break the contract. In that case the impact would be high, and this issue critical.

Fix

Only store spinner identifiers in omegaSpinners. This has the added benefit of reducing storage cost.

[nickjm]

Fixed in https://github.com/nickjm/cryptospinners-bounty/pull/3

Will finalize award soon.