Pack sales can be rigged to get specific cards

[AusIV]

Description

If a user wants to purchase a pack and ensure they get a specific card, or a specific class of card, they can write a contract that ensures they only pay for a pack if it has the cards they want.

Scenario

A contract calls buyPackWithETH, checks the result to see if the desired card has been received, and reverts the transaction if they didn't get the card or class of card it wants. In this situation the user has to pay the gas for the transaction whether or not they get the card they want, but they only have to pay for the pack if it has desirable cards.

Impact

Users who purchase premium packs through tailored contracts can avoid paying for the pack if it doesn't have an iconic card. This will remove those packs from circulation while leaving packs that don't include iconic cards, making it unlikely that users buying premium packs through normal means can get iconic cards.

Outside of premium sales, such contracts could guarantee that they get diamond cards, and revert the transaction for packs that only have lower quality cards.

Reproduction

At this point I haven't written a contract to exploit this, but could do so if required to claim a bounty. I believe my explanation above is sufficient to illustrate the problem.

Fix

As you've noted in some of your comments, randomness in contracts is problematic. Because contracts are capable of reverting transactions if they don't get the outcome they want, anything involving randomness needs to happen in two phases, commit and reveal. In the commit phase, the user pays their fee and commits to a particular random value. In the reveal phase, the player's reward is assigned. This way if the player is a contract, it cannot revert the commit phase if the reveal phase doesn't provide the desired reward.

If it is critical that the pack sale happen in a single transaction, the only real option is to bar contracts from making purchases. This can be done with require(msg.sender == tx.origin). If msg.sender differs from tx.origin, then msg.sender is a contract and might be programmed to cheat. Unfortunately, this also prevents things like multisig contracts from playing, and once account abstraction is introduced it may prevent many legitimate contracts from being able to purchase packs. It would not be necessary to bar contracts from all participation in CryptoStrikers, just in the operations dependent on random outcomes. They could still own and trade cards, but not purchase packs.

Neither the commit / reveal approach nor blocking contracts from making purchases makes your randomness totally safe. Miners in particular could potentially manipulate the timestamp or other factors to give themselves a win. But the current implementation would allow anyone capable of writing a contract to ensure that they only pay for a pack if it has cards they wanted.

[giaset]

Thanks for the extremely well written and thorough issue -- definitely agree that this is a problem. Need to discuss a few things with the team, is there anywhere we can reach you to chat directly?

[AusIV]

I'm about to head to bed, but I reached out to you on Gitter. I'll provide contact information there.

[giaset]

Chatted with @AusIV on Gitter, and we've awarded him the following reward for this issue.

Impact: Medium Likelihood: High Severity: High Points: 500

The fix we will be implementing is:

Preventing smart contracts from purchasing packs, by adding a require(msg.sender == tx.origin) check to _buyPack().