dapperlabs/cryptokitties-bounty

Bounty program for CryptoKitties smart contract

The CryptoKitty Bounty Program is NOW CLOSED

Thank you everyone for your participation!! Please stay tuned for rewards!

CryptoKitty Bounty Program CryptoKitties recognizes the importance of security researchers in keeping our community safe and fun. With the launch of CryptoKitties around the corner, we would love the community to help provide disclosure of security vulnerabilities via our bounty program described below.

What you Should Know About CryptoKitties:

• CryptoKitties is a game centered around breedable, collectible, and oh-so-adorable creatures we call CryptoKitties! Each cat is one-of-a-kind and 100% owned by you; it cannot be replicated, taken away, or destroyed.
• CryptoKitties are non-fungible tokens (see ERC #721) that are indivisible and unique.
• The smart contracts have over 4-billion variations of phenotypes (what you see) and genotypes (what you don’t see). Because cats are tokens on a blockchain, they can be bought, sold, or transferred digitally, with strong guarantees of ownership.
• CryptoKitties is built on the Ethereum network; ether will be necessary to fuel transactions, which include purchasing, trading, and breeding CryptoKitties. For the purpose of this bounty program, the program will run within the Rinkeby test network. Also the source code will be available for review.
• Two CryptoKitties can breed a new CryptoKitty. Kitties do not have a permanently assigned gender. While they can only engage in one breeding session at one time, each Kitty is able to act as either matron or sire. The owner chooses per breeding interaction!

See full basic operations here

The Scope for this Bounty Program:

This bounty program will run within the Rinkeby network from 12:01am GMT November 16th - 11:59pm GMT November 20th, 2017. All code important to this bounty program is publicly available within this repo Help us identify bugs, vulnerabilities, and exploits in the smart contract such as:

• Breaking the game (ex. auctioning doesn’t work, breeding doesn’t work,)
• Incorrect usage of the game
• Steal a cat from someone else
• Act as one of the admin accounts
• Complete something without respecting the cool-down period
• Any sort of malfunction

Rules & Rewards:

• Issues that have already been submitted by another user or are already known to the CryptoKitty team are not eligible for bounty rewards.
• Bugs and vulnerabilities should only be found using accounts you own and create. Please respect third party applications and understand that an exploit that is not specific to the CryptoKitty smart contract is not part of the bounty program. Attacks on the network that result in bad behaviour are not allowed.
• The CryptoKitty website is not part of the bounty program, only the smart contract code included in this repo.
• The CryptoKitty bounty program considers a number of variables in determining rewards. Determinations of eligibility, score and all terms related to a reward are at the sole and final discretion of CryptoKitty team.
• Reports will only be accepted via GitHub issues submitted to this repo.
• In general, please investigate and report bugs in a way that makes a reasonable, good faith effort not to be disruptive or harmful to us or others.

The value of rewards paid out will vary depending on Severity which is calculated based on Impact and Likelihood as followed by OWASP:

Note: Rewards are at the sole discretion of the CK Team. 1 point currently corresponds to 1 USD (paid in ETH) The top 10 people on our leaderboard of accepted bugs with at least 250 points will receive a limited edition BugCat available only to successful participants in this bounty program.

• Critical: up to 1000 points
• High: up to 500 points
• Medium: up to 250 points
• Low: up to 125 points
• Note: up to 50 points

Examples of Impact:

• High: Steal a kitty from someone, steal/redirect ETH or kitties to another address, set kitty genes to arbitrary value, block actions for all users or some non-trivial fraction of users, create a kitty without breeding.
• Medium: Break breeding rules (self-breeding, sibling or parent breeding, by-pass cooldowns), lock a single kitty owned by an address you don't control, manipulate the gen0 auction price without buying gen0 kittens.
• Low: Block a user from bidding during an auction, create price or comission errors in auction, cancel or block another user's auction.

Suggestions for Getting the Highest Score:

• Description: Be clear in describing the vulnerability or bug. Ex. share code scripts, screenshots or detailed descriptions.
• Fix it: if you can suggest how we fix this issue in an appropriate manner, higher points will be rewarded.

Cryptokitties appreciates you taking the time to participate in our program, which is why we’ve created rules for us too:

• We will respond as quickly as we can to your submission (within 3 days).
• Let you know if your submission will qualify for a bounty (or not) within 7 business days.
• We will keep you updated as we work to fix the bug you submitted.
• CryptoKitties' core development team, employees and all other people paid by the CryptoKitties project, are not eligible for rewards.

How to Create a Good Vulnerability Submission:

• Description: A brief description of the vulnerability
• Scenario: A description of the requirements for the vulnerability to happen
• Impact: The result of the vulnerability and what or who can be affected
• Reproduction: Provide the exact steps on how to reproduce this vulnerability on a new contract, and if possible, point to specific tx hashes or accounts used.
• Note: If we can't reproduce with given instructions then a (Truffle) test case will be required.
• Fix: If applies, what would would you do to fix this

FAQ:

• How are the bounties paid out?
  • Rewards are paid out in ETH after the submission has been validated, usually a few days later. Please provide your ETH address.
• I reported an issue but have not received a response!
  • We aim to respond to submissions as fast as possible. Feel free to email us if you have not received a response.
• Can I use this code elsewhere?
  • No. Please do not copy this code for other purposes than reviewing it.
• I have more questions!
  • Create a new issue with the title starting as “QUESTION”
• Will the code change during the bounty?
  • Yes, as issues are reported we will update the code as soon as possible. Please make sure your bugs are reported against the latest versions of the published code.
• I'm having trouble setting up the contracts?
  • Join the CryptoKitties Gitter to get some assistance

Important Legal Information:

The bug bounty program is an experimental rewards program for our community to encourage and reward those who are helping us to improve CryptoKitties. You should know that we can close the program at any time, and rewards are at the sole discretion of the CryptoKitties team. All rewards are subject to applicable law and thus applicable taxes. Don't target our physical security measures, or attempt to use social engineering, spam, distributed denial of service (DDOS) attacks, etc. Lastly, your testing must not violate any law or compromise any data that is not yours.

Copyright (c) 2017 Launch Labs, Inc. (dba Axiom Zen)

All rights reserved. The contents of this repository is provided for review and educational purposes ONLY. You MAY NOT use, copy, distribute, or modify this software without express written permission from Axiom Zen or Launch Labs, Inc.

cryptokitties-bounty's Issues

Gas inefficiencies in `KittyMinting`

Description

There are a couple of gas inefficiencies in KittyMinting that could be resolved for significantly lower gas consumption:

• promoCreationLimit and gen0CreationLimit are both defined as variables. Making them constant would allow the compiler to optimise them out, saving 40k of deployment-time gas, and ~200 gas each time they are read.
• createPromoKitty increments both variables, costing ~10k. Incrementing only promoCreatedCount, and checking the sum of the two variables instead of each separately, would save ~5k gas at update time.

Final Leaderboard

The winners of the CryptoKitty bounty are as followed:

@Arachnid 325 points
@adamkolar 225 points
@pauliax 175 points

• Please send us the ETH address you wish to receive your funds to either by commenting on this issue or emailing me directly at kim @ axiomzen.co.
• Please also send us your ETH address you wish to receive your very own BugCat, which will become available to you in a few weeks.

Thank you so much for your participation in our bounty program!

yy

##@##

Description

A brief description of the vulnerability, why you think this is a bug.

Scenario

A description of the requirements for the vulnerability to happen

Impact

The result of the vulnerability and what or who can be affected

Reproduction

Provide the exact steps on how to reproduce this vulnerability on a new contract, and if applies, point to specific transaction hashes or accounts used.
A test case is always welcome, and it might be required for acceptance.

Fix

If it applies, describe what would would you do to fix this. Pull requests are disallowed.

unbounded loop

function totalSupply() public view returns (uint) {
    return kitties.length - 1;
}

for (uint256 i = 1; i <= totalSupply(); i++) {...}

This loop in the contract can eventually grow so large as to make future operations of the contract cost too much gas to fit in a block. Then, iterating through this large array might consume all the gas provided (run out of gas) and it will never complete.

Code is open source

This comment suggests that CK is open source (except for gene splicing): https://github.com/axiomzen/cryptokitties-bounty/blob/e0d9c2c90964b1bbb242d8e3e0d9a7786cf21182/contracts/KittyCore.sol#L16-L17

But the license file contracts this: https://github.com/axiomzen/cryptokitties-bounty/blob/master/LICENSE.md

So is this code open source or not?

Extra value in bids is silently kept by the auction contract

Description

SaleClockAuction.sol:58 places the bid and computes the price, but does nothing with the price other than record it if the auction is a gen0 auction.

_bid in ClockAuctionBase:104 requires that the bid amount be at least as high as the current price. It then transfers the current price less the auctioneer's cut to the seller, and (implicitly) keeps the remainder.

In the case that the amount sent to the contract is more than the current price, this results in the auction contract keeping not just the auctioneer's cut, but also any excess funds. Instead, any excess should be returned to the buyer.

Scenario

Any time a user bids on an auction with a larger than required amount, they will be shortchanged by the auction contract. Because the dutch auction model continuously reduces the price, and because there is an inevitable delay between sending a transaction and it being mined, there will almost always be excess funds.

Impact

All bidders can be shortchanged on sale prices.

Reproduction

See "description" and "scenario".

Fix

Modify either _bid or bid to return excess funds to the caller.

Listing all kitties owned by a user is O(n^2)

Description

tokensOfOwnerByIndex linearly scans all kitties looking for the nth one owned by the specified account. Since each call iterates over all the previous results, if there are n kitties, m of them owned by the specified address, this will take O(m * n) time.

Although this function notes that it's not intended to be called onchain, even offchain the execution time of listing kitties will soon become impractical.

Scenario

1. Create lots of kitties.
2. Attempt to enumerate all kitties owned by someone with lots of kitties.

Impact

It may become effectively impossible to list someone's kitties - particularly newer ones.

Reproduction

See Scenario.

Fix

Maintain an array of kitties-by-owner. This array can be efficiently updated by moving the last element into deleted indexes, since it's not necessary for the array to be kept in order. EG:

mapping(address=>uint[]) kittiesByOwner;

function deleteKitty(address owner, uint idx) {
  var kittyList = kittiesByOwner[owner];
  require(idx < kittyList.length);
  if(idx < kittyList.length - 1) {
    kittyList[idx] = kittyList[kittyList.length - 1];
  }
  kittyList.length -= 1;
}

Possible alternative to rescueLostKitty function

Is there a reason why transferral to the address of the ERC721 contract isn't forbidden in the _transfer function instead of solving the problem with the rescueLostKitty function? It's a question of one require, so the added gas cost should be pretty minimal and considering the amount of time and energy needed to coordinate a "rescue mission", it could be worth it.

    /// @dev Transfers a kitty owned by this contract to the specified address.
    ///  Used to rescue lost kitties. (There is no "proper" flow where this contract
    ///  should be the owner of any Kitty. This function exists for us to reassign
    ///  the ownership of Kitties that users may have accidentally sent to our address.)
    /// @param _kittyId - ID of kitty
    /// @param _recipient - Address to send the cat to
    function rescueLostKitty(uint256 _kittyId, address _recipient) public onlyCOO whenNotPaused {
        require(_owns(this, _kittyId));
        _transfer(this, _recipient, _kittyId);
    }

Add Checks to ensure 'C' level role addresses are updated with different values

Description

There is no check available in setCEO / setCFO / setCOO methods to ensure that the incoming address is different from the current address of the respective role. By adding this additional check, any attempt to re-assign the same address value to a particular role can be avoided, saving gas cost.

Scenario

CEO address is 0x16baf0de678e52367adc69fd067e5edd1d33e00f. CEO invokes with the same address value setCEO("0x16baf0de678e52367adc69fd067e5edd1d33e00f"). This transaction would go through because there is no check to validate if the incoming address is not same as the current address.

Same applies for the setCOO & set CFO methods.

Impact

No real attack vector but possibility to save some gas.

Reproduction

Refer Scenario above.

Fix

Add require statement to ensure that the incoming address is different from the current address.

function setCEO(address _newCEO) public onlyCEO {
require(_newCEO != ceoAddress);
require(_newCEO != address(0));

    ceoAddress = _newCEO;
}

Similar checks can be applied for the setCOO & set CFO methods.

Auction DoS possibility still problematic

This comment in ClockAuctionBase:

            // NOTE: Doing a transfer() in the middle of a complex
            // method like this is generally discouraged because of
            // reentrancy attacks and DoS attacks if the seller is
            // a contract with an invalid fallback function. We explicitly
            // guard against reentrancy attacks by removing the auction
            // before calling transfer(), and the only thing the seller
            // can DoS is the sale of their own asset! (And if it's an
            // accident, they can call cancelAuction(). )
            seller.transfer(sellerProceeds);

correctly states that seller can only DoS sale of his own asset, but I think this is still problematic. It's true that contract that owns the kitty on sale can merely revert transaction by throwing, but this still imposes gas cost on buyer who attempted to place a valid bid. And since there is no easy way to discern good auctions from fake ones, a griefer can create a minefield which could hamper smooth trading. I think using withdraw pattern instead of immediate transfer is preferable.

Typo in the CryptoKitties_WhitePapurr_V2

Description

Not a vuln but a tiny typo in your awesome WhitePapurr, Conclusion: The Future is Meow section:
and any any future blockchain projects -> and any future blockchain projects

check in modifier (canBeStoredWith128Bits)

The check in this modifier should be "<="

modifier canBeStoredWith128Bits(uint256 _value) {
    require(_value < 340282366920938463463374607431768211455);
    _;
}

because 128 bits can hold a maximum of 2^128-1 which is equal to 340282366920938463463374607431768211455 so it makes sense (according to the modifier's name) to have a "<=" instead of "<".

by the way, here everything is fine:
modifier canBeStoredWith64Bits(uint256 _value) {
require(_value <= 18446744073709551615);
_;
}

Hardcoded numbers

    require(_matronId <= 4294967295);
    require(_sireId <= 4294967295);
    require(_generation <= 65535);

// ... some other code...

    require(newKittenId <= 4294967295);

It's generally advised to avoid hardcoded numbers in functions. Functions shouldn't have magic numbers inside it. It makes harder to maintain the code and also decreases readability. Imagine if you use a magic number, let's say 350 ten times in your code. One day you decide to change it from 350 to 360. What do you do? You search all occurrences in the code and change them. If you have this value extracted, you can change it only in one place to achieve the same effect.

Functions' visibility

Consider marking functions with a specific access level, for example, functions that aren't called internally, please mark with a keyword "external". Calling external function consumes less gas than public function. Also, functions that are only used internally, should be marked "internal". If you want, I can specify which modifier every function should have.
As for best practices, you should use external if you expect that the function will only ever be called externally, and use public if you also need to call this function internally.

Now, in your code ~90% of the functions are public, despite some of them are only called from the outside world.

Limit gasPrice in auctions

Description

People are using very high gas price to get advantage in auctions, and leading everyone to also have to rise their gasPrices.

Impact

Network congestion

Fix

Require a limit to gasPrice of 5 gwei in auctions modifieres or equivalent.

Possibly missing implementation of intended functionality

This comment:

        // Set to the index in the cooldown array (see below) that represents
        // the current cooldown duration for this Kitty. This starts at zero
        // for gen0 cats, and is initialized to floor(generation/2) for others.
        // Incremented by one for each successful breeding action, regardless
        // of whether this cat is acting as matron or sire.
        uint16 cooldownIndex;

says that cooldownIndex should be initialized to floor(generation/2) for all kitties with generation > 0. However I haven't found any code that implements this functionality, all kitties regardless of generation seem to start at 0.

setCEO error

await testInstance.setCEO("0x200611411221535804aa6f053c5fcc78ef1354d5");
Error: invalid address throw new Error('invalid address');

accounts:
Array [ "0x9556d2d19f5195454bf6034221bb20477f716bc5", "0xd75ccf594ed62dff1771635d310d02f013384c6b", "0xe77603a978adb27ac4a282edc4fcb0443847d24e", "0x3ba1d1bfc848c5174713710a0f94305f93922e6f", "0x922aacf435e4d9c767b7b0360e5ab702b4884b6f", "0x615345b4d999942a7ee30b54b72ee83fe82a62a5", "0xfa9e19079b0e90e36e5410bca6d1d5b39cbb4779", "0x870d0e5a84cb8120ef99a391c9430ede6496db1f", "0xf24eeec90b57da68325e9750a0972113c4ef1100", "0x200611411221535804aa6f053c5fcc78ef1354d5" ]

CEO can take over the roles of CFO / COO

Description

CEO can take over the roles of CFO / COO

Scenario

The CEO is the only role that has permissions to change the values of the CFO / COO. But there is no check in place to ensure that the CEO doesn't take over all 'C' roles.

Impact

CEO monopolises the control of the 'C' roles and the contract.

Reproduction

See above.

Fix

Add require statements to ensure that _newCFO / _newCOO values are not equal to ceoAddress.

`rescueLostKitty` can be replaced with a check

Description

rescueLostKitty exists to restore ownership of kitties accidentally transferred to the token contract itself. This can be replaced by a simple check on transfer that prevents this happening in the first place.

Scenario

1. User transfers a kitty to the token contract
2. rescueLostKitty is called.

Impact

Some inconvenience for the user and for the maintainers of the contract.

Reproduction

See Scenario.

Fix

Add a check require(to != address(this)) in the transfer function.

**Recommendation** Adding events to track 'C' level updates

Description

Updates for 'C' level role changes don't fire events, its a good idea to have this in place to trace all significant events that happen on the contract.

Reproduction

Invoking a COO / CFO address update action by the CEO does not trigger events.

Fix

Add events to indicate that the COO / CFO role addresses have been updated.

0x0ae4cAcDe6b5a0d31369b311658e5897ED6b9938

0x0ae4cAcDe6b5a0d31369b311658e5897ED6b9938

Originally posted by @kophyo1234 in trustwallet/assets#18068 (review)

CryptoKitties Bounty Is Now CLOSED!

Thank you so much to all participants in our bounty! There were a few good bugs and lots of great suggestions that we will be taking into consideration. To all those participants please expect to hear back from us later this week with information regarding your rewards and personalized bug cats!

Empty fallback in `ClockAuctionBase` has unintended consequences

Description

ClockAuctionBase:39 defines an empty fallback function. This overrides Solidity's default, which is a fallback function that always reverts.

Solidity's default already prohibits sending ether to the contract. By overriding the default with a function that does not throw or revert, this ensures calls to functions the contract does not implement will silently return instead of throwing.

Scenario

One example scenario is the tokenFallback of ERC223. This function is called on contracts when tokens are sent to them in order to avoid lost tokens, and it is expected that they throw if they do not want to accept tokens. This contract, by virtue of having an empty fallback, will silently accept (and trap) ERC223 token transfers.

Impact

Anyone attempting to call nonexistent functions on this contract will get a silently successful result with empty return data. Generally this is harmless, but in situations where someone is expecting the contract to implement a common interface, such as the scenario above, it may lead to lost funds.

Reproduction

See 'Scenario' above.

Fix

Remove the fallback function.

0x0ae4cAcDe6b5a0d31369b311658e5897ED6b9938

Description

A brief description of the vulnerability, why you think this is a bug.

Scenario

A description of the requirements for the vulnerability to happen

Impact

The result of the vulnerability and what or who can be affected

Reproduction

Provide the exact steps on how to reproduce this vulnerability on a new contract, and if applies, point to specific transaction hashes or accounts used.
A test case is always welcome, and it might be required for acceptance.

Fix

If it applies, describe what would would you do to fix this. Pull requests are disallowed.

bid amount: comment vs code

    // Check that the incoming bid is higher than the current
    // price
    uint256 price = _currentPrice(auction);
    require(_bidAmount >= price);

The comment says that the bid should be higher, however, the code also allows equal value, so please either refactor the code or update the comment.

Indexed parameters

Consider adding "indexed" keyword next to the event parameter. The indexed parameters allow filtering events by specific addresses. For example,
event AuctionCreated(uint256 tokenId, uint256 startingPrice, uint256 endingPrice, uint256 duration);
//make tokenId indexed
event AuctionSuccessful(uint256 tokenId, uint256 totalPrice, address winner);
//make winner indexed
event AuctionCancelled(uint256 tokenId);
//make tokenId indexed
event Pregnant(address owner, uint256 matronId, uint256 sireId);
//make owner indexed

CEO could transfer entire balance to a seller

Description

There are actually not full guarantees that the cut is no greater than 100%. A complex auction contract could overwrite the ownerCut state to a number that sends the whole balance of the auction to a seller.

Scenario

CEO sets an auction (either sire or selling) that overwrites the ownerCut state after the require(_cut <= 10000) in the ClockAuction constructor has completed. This would presumably be an accident, but could also be a malicious CEO who is also the seller. The transfer:
https://github.com/axiomzen/cryptokitties-bounty/blob/282e43bcefad4b9446a281855eef7d63495c2d53/contracts/Auction/ClockAuctionBase.sol#L148
would be a very large number due to storing a negative number in an unsigned integer. It could be the exact balance of the auction.

Impact

Seller drains auction account(s) and can continue to drain them by placing more auctions.

Reproduction

function WhoopsClockAuction(address _nftAddr, uint256 _cut) public ClockAuction(_nftAddr, _cut) {
  //the require(_cut <= 10000) has completed at this point
  ownerCut = 9999999999;  //malicious value.
}

The bad state could also be caused by "complex math" in the future. i.e. you want to autonomously continuously deploy new contracts that have different cuts based on some external factors. If there is a mistake in this math, the number could be bad. I think malicious is more likely but who knows how complex futures contracts could be?

Fix

Add a require(ownerCut <= 10000) in function _computeCut(uint256 _price).

Reason behind short-term stable randomness for GeneScience contract

Description

The gene science contract use a blockhash to produce some sort of random mutation if my understanding is correct. From what I can see, it uses the following logic:

func blockhash(p) {
    if (currentBlockNumber - p < 256) 
        return hash(p);
    return 0;
}

var bhash = blockhash(thrid);
if (bhash == 0) {
    thirdProjection = (currentBlockNumber & ~0xff) + (thridParam & 0xff);
    if (thirdProjection > currentBlockNumber) {
        thirdProjection -= 256;
    }
    thirdParam = thirdProjection;
    bhash = blockhash(thirdProjection);
}

I am curious to know the reasoning behind this. Or if we are totally missing the point and made some big mistakes reading the bytecode.

Scenario

A pregnant matron and some spare time..

Impact

Create unfairness due to easy-to-predict gene mutation and players may game the system by waiting for a good mutation.

Reproduction

https://www.reddit.com/r/ethereum/comments/7ivuyf/towards_cracking_kitties_genetic_code_or_a_quick/?st=jb1j16ui&sh=9d19ac63

Fix

You got this one developed: https://github.com/axiomzen/eth-random, so I am thinking this must be a business decision to use that short-term-stable genetic mutation logic. I don't think there is any fix needed if that is intended behavior, just curious.

When kitty gives birth in the ownership of SaleClockAuction, offspring becomes locked

If birth happens while the kitty is in the ownership of SaleClockAuction contract, the offspring stays in the ownership of the auction contract indefinitely.
As a solution, I propose adding require to giveBirth function that checks if the current owner isn't SaleClockAuction contract, second option is preventing pregnant kitties from being put up for sale.