etherbots / etherbots-bug-bounty

Code implemented on Rinkeby testnet, along with a wiki guide for the Bug Bounty.

ETHERBOTS Bug Bounty

ETHERBOTS is a decentralised Robot Wars game on the Ethereum blockchain. It is currently live on the Rinkeby testnet at https://etherbots.io/. This wiki will step you through a high level overview of the code, and set out a criteria for submitting bugs. The github code will be updated frequently as we respond to bugs, so make sure to pull the latest versions. We highly recommend cloning/forking a version of this repository so you have access to linter & build functionalities for your convenience.

The mainnet game launches on March 3 PST.

INTRODUCTION TO ETHERBOTS

• ETHERBOTS is a game where you can fight, collect and trade Robots.
• Each part is a non-fungible ERC721 token, fully owned by users.
• Parts can be gained in several ways.
• 1. Battling provides a number of shards based on your performance in battle and a random modifier. These shards can be redeemed for parts.
• 2. Battling can randomly give you a part as an immediate reward, if you are very lucky.
• 3. Parts can be bought and sold on the user run marketplace.
• 4. Parts can be "scrapped" to redeem 70% of the shard value of a part crate.

Some users already have parts, which they bought during the presale. These users will migrate their parts into full ERC721 tokens, a process coded in EtherbotsMigrations.sol.

BUG BOUNTY

This bounty will run within the Rinkeby network from 11.59 PM PST February 23 - 11:59pm PST March 2, 2018.

The initial fund allocated to this Bounty is 10 Ether. However we will update the wiki if this is exhausted, and allocate more funds as required.

Our main area of concerns are

• Anything which breaks the game or makes it non-functional (getting robots "stuck" in battles, etc.)
• Stealing other people's robot parts
• Gaining an unfair advantage over other people (accessing defender's secret commits without social engineering)
• Stealing ETH from any of the contracts or users
• Illegitimately using an onlyOwner function or changing state in a way regular users shouldn't be allowed to
• Errors in programming/game logic which would impact gameplay
• Gas efficiency, particularly in the battle contract (_executeMoves function)

Rules

1. Issues will only be considered as valid if they have not been submitted by another user, or are not known to the Etherbots team.
2. Please stay legal. No DDOSing, no social engineering, etc. The live game on the Etherbots website is a useful way to interact with the contract as users would, but it is not part of the bounty itself.
3. We are interested in economic logic or game logic errors to do with the expected value of battling, reward crates, etc. While difficult to quantify, if you make a reasonable suggestion based on a serious error of ours which we implement, we will reward it.
4. Please submit all reports via GitHub issues on this repository in order to be eligible for rewards.
5. Final note: all rewards is at the sole discretion of the ETHERBOTS team. We will conduct this bounty in very good faith, as the security of our user's parts and game experience is paramount to us. We ask you do the same, and don't disrupt user's testnet experience or act in a harmful manner. <3

REWARD CRITERION

Rewards will be determined according a risk matrix of impact severity and likelihood, defined by OWASP.

1 point = 1 USD worth of ETH at time of payment.

• Critical: up to 1000 points

Examples: Steal someone's part trivially. Steal ETH off a user or the contract. Access onlyOwner functions. Ruin a significant portion of game experience.

• High: up to 500 points

Examples: Access things for free, i.e. battling without battle fees. Control any functions' randomness in an exploitable way.

• Medium: up to 250 points

Examples: Disrupt other user's actions, such as stopping them from being able to bid on an auction, stopping them from battling or revealing moves, etc.

• Low: up to 100 points

Examples: Scaling issues -- anything that might break with large amounts of users. Improvements in gas efficiency (we will consider proportionate to gas saved -- if highly significant and a specific fix is recommended, we will award higher points than this cap. Keep in mind the functionality needs to remain the same.) Break the strategic integrity of the game (i.e. find a way to collude beneficially).

Please be clear in your description of the bug. Recreate the exact steps needed to make the bug happen with screenshots, code, a video or descriptions.

For higher points, suggest how we can fix the bug.

Rules for Etherbots

• We will respond as promptly as possible to all bug submissions.
• We'll let you know if your bug qualified for a bounty within a business week.
• No Etherbots team members, developers or contractors are eligible for bounties.

Disclaimer: This bug bounty is an attempt by the ETHERBOTS team to ensure the highest level of security and seamlessness in the final game experience. The code published here is purely for testing purposes and is not open source or available for private or commercial use without our express permission.

Copyright (c) 2018 Fuel Bros Pty, Ltd. All rights reserved.

etherbots-bug-bounty's Issues

A method to lock bots or force attackers to win

This is partly related to #3 (and I found this via that issue), but this is a separate and more serious problem.

Currently, attack doesn't validate the type or count of the attacker's parts; it only checks that the attack owns them all, if any (ownsAll will return true even if the attacker is attacking with 0 parts). Here are some implications:

• The attacker doesn't need 1 of each of the 4 types of parts. This clearly breaks the rock-paper-scissors style of the game, and also allows the attacker to send one part 4 times (since this isn't checked either)

• If the attacker sends less than 4 parts, then defenderRevealMoves will always cause an array out of bounds exception, specifically in _executeMoves when attackerParts is constructed (line 651). Because of this, the defender cannot win, and they will either have to call cancelBattle or an attacker can call claimTimeVictory.

• If the attacker sends 0 parts or doesn't choose their moves advantageously, then cancelBattle and claimTimeVictory will also cause an array out of bound exceptions in _forfeitBattle -> _allocateExperience -> exps[moves[i]] (line 856). This causes all the defender bot to be locked up, as the duels can't be resolved and the bot transferred back.

• If the attacker sends 1-3 parts and does choose their moves advantageously (sending only a DEFENCE and MELEE part, and only DEFENDing, for instance), then cancelBattle and claimTimeVictory will not cause an exception on line 856, and the rest of the experience code will not crash. This means that all attackers can be forced to win.

• If the attacker sends more than 4 parts, then most of the battle logic works properly, although the experience scaling is not correct and additional legitimate attackers will likely not be able to attack because _canAttack will cause an out of bounds exception.

Fix:

The attacker parts should also be validated like they are for the defender:

// check parts //defence1 melee2 body3 turret4
uint8[4] memory types = [1, 2, 3, 4];
require(_base.hasPartTypes(partIds, types));

Mech overrides Android

In the _applyBonusTree function in StageBattle.sol, the else if's prevent Android perks from being considered if the user also has the Mech perk. For eaxmple, if the user has filled out the perk tree entirely, then it is impossible to get any perk bonuses for fire or water type moves, because the Mech clause will only check to see if the move was electric or steel.

I believe this could be fixed by swapping the order of the conditionals, similar to how the code handles offense/defense and the different moves.

Multiple charges for reward crate

I'm not exactly sure if this qualifies as a bug, as I don't believe I received more than the initial crate reward, but the "claim reward" button was continuously clickable, and I was able to queue up a dozen transactions. It was only after I exited the browser and reloaded it that the button disappeared

Bots get locked if transaction is stuck

This situation is currently quite common:

The main reason as far as I understand is: they cant reveal the secret moves is because their robot is stuck in the transaction failure.
You can't forfeit either, here's screenshot of the forfeit problem, its not possible to cancel the battle, nor fight with the attackers.

lowering the gas limit, so the transaction can get through results in a failed tx

Effectively the bots are stuck in limbo.

Potential Dutch Auction Issue?

I mostly know Fortran (ya, I know...), but I ran into an issue in the market and thought I'd take a stab at finding it.

I noticed that whenever I tried to buy a part in the marketplace that was selling with a Dutch Auction, my tx would fail. I was able to buy Static Price parts no problem. From looking at the code, my thought is that the price the buyer is attempting to purchase at is not aligning with the price that the code is attempting to calculate due to the delay between sending a tx to the blockchain and when it's actually executed. So, my thought is that in GenericAuction.sol , _purchaseAmount is the value that the buy thinks she's paying when starting the tx. The actual price is calculated from

    function _currentPrice(Auction storage _auction) internal view returns (uint256) {
        uint256 secsElapsed = now - _auction.start;
        return _computeCurrentPrice(
            _auction.startPrice,
            _auction.endPrice,
            _auction.duration,
            secsElapsed
        );
    }

where I'm guessing now is the time that this code is executed. This causes a failure when

        require(_purchaseAmount >= price);

is checked because if price is increasing with time, _purchaseAmount is no longer greater than price.

I guess my solution would be to pass a _currentTime variable when _purchaseAmount is set so that the price calculated in _currentPrice doesn't fluctuate. This could replace now. Unless it's the care where that's what now is already doing because I can't seem to figure out what now is defined as. Haha

In order for EVM to optimize the read write of storage, pack variables as tightly as possible.

http://solidity.readthedocs.io/en/develop/miscellaneous.html

This can be seen in struct Part.
struct Part {
uint32 tokenId;
uint8 partType;
uint8 partSubType;
uint8 rarity;
uint8 element;
uint32 battlesLastDay;
uint32 experience;
uint32 forgeTime; // TODO: check whether we need uint64
uint32 battlesLastReset;
}

tokenId should be next to battlesLastDay in order to save on gas.

Battle screen improvements

1. Impossible to see artwork in the top/center of the screen, because it overlaps.

2. See some of the suggestions for battle screen UI. It would be great to add:
   — Information about the total moves won by each party
   — Overall battle indicator - how close is the bot to winning or losing
   — Total damage inflicted, as moves won don't necessarily determine the outcome.

I can provide a more well thought-through mockup if needed.

Nothing happens when clicking 'N Shards' button upon opening a crate

originally reported this as jorjik on Discord

Optimized version of `_applyBonusTree`

The following is an optimized version of the _applyBonusTree function, which takes advantage of the binary tree structure of the perk tree. The PT constants are no longer necessary with this version, and the helper functions getMoveType, hasPerk, and _applyPerkBonus can all easily be made inline and removed. This version also fixes #1 and #2.

bytes4 constant moveToPT = 0x06050304;
bytes4 constant elementToPT = 0x05060403;
uint8 constant PRESTIGE_BONUS = 1;

function _applyBonusTree(uint8 move, EtherbotsBase.Part[4] parts, uint8[32] tree) internal pure returns (uint8 bonus) {
    uint8 prestige = tree[PT_PRESTIGE_INDEX];
    
    uint8 active = 0;
    uint8 level2 = uint8(moveToPT[move]);
    uint8 level1 = (level2 - 1) / 2;
    
    if (tree[level1] > 0) {
        active++;
        if (tree[level2] > 0) {
            active++;
            
            uint8 level4 = level2 * 4 + uint8(elementToPT[parts[move].element]);
            uint8 level3 = (level4 - 1) / 2;
            
            if (tree[level3] > 0) {
                active++;
                if (tree[level4] > 0) {
                    active++;
                }
            }
        }
    }

    bonus = active * (PERK_BONUS + (prestige * PRESTIGE_BONUS));
}

Defender's moves aren't validated

Unlike the attacker, the defender's moves aren't currently being checked for the proper length or range in defenderRevealMoves.

If the defender's doesn't supply enough moves or the first MOVE_LENGTH moves aren't valid, then this will always cause an out of bounds exception in defenderRevealMoves -> _executeMoves if there are not enough moves and in defenderRevealMoves -> _executeMoves -> _applyBonuses -> getPartRarity if any of the first MOVE_LENGTH moves is not 0-3.

If the defender supplies more than MOVE_LENGTH moves, then there seem to be no ill effects, as only the first MOVE_LENGTH are considered.

Due to the nature of the commit/reveal scheme, we cannot check the validity of the defender's moves in _defenderCommitMoves, so if the defender originally committed with invalid moves, then they will be forced to either cancel the battle or let it time out.

Fix:

When the defender does reveal their moves, it might be worth considering nullifying the duel if the moves weren't valid to begin with.

Even if this approach isn't taken, this can still be a lurking bug for future battle implementations and it would save gas if require(_isValidMoves(_moves)) was added to defenderRevealMoves to avoid exceptions.

Incorrectly using parts.length as the new part id

Throughout the contracts you store parts in an array parts. Since you are using an array, this is a 0 based indexed list. In the forge when you create a part, you set the id to be equal to parts.length. , while in other places such as the experience awards you add experience to the part in the part[_id] index. Because an array always has length - 1 indices, this is awarding experience incorrectly to the part one index after the part that should have the experience awarded.

Ex: With 99 parts in existence, creating a new part would give it the tokenId 100, but parts[100] would not return an existing part.

Places where this issue exists:
Base.sol#L153
ERC721.sol#338
ERC721.sol#397
EtherbotsBattle.sol#L229
EtherbotsBattle.sol#L229 (2x)
EtherbotsBattle.sol#L268

Likelihood: High
Impact: High

Ethereum Address: 0x3D9317cC2e741794C8b53E175e2C1e28170F6e18

OwnsAll returns true for when an empty partIds array is provided

Still a way off of understanding the underlining impact of this as I need to piece together a bit more but I believe there's several external functions which can be called by a user directly and by not passing in any _tokenIds (partIds) they'll almost certainly bypass the ownsAll conditional check and may default to owning part at index 0 (owned by another user) further down the line when an out of bounds index is called.

Current ownsAll implementation

function ownsAll(address _owner, uint256[] _tokenIds) public view returns (bool) {
    for (uint i = 0; i < _tokenIds.length; i++) {
        if (partIndexToOwner[_tokenIds[i]] != _owner) {
            return false;
        }
    }
    return true;
}

Impacted functions:

function createBattle(uint _battleId, uint[] partIds, bytes32 commit) external payable
function attack(uint _duelId, uint[] parts, uint8[] _moves) external payable returns(bool)

Not impacted but prone to errors:
function transferAll(address _to, uint256[] _tokenIds) public whenNotPaused

Fix
You should revert if an empty array of partIds/tokenIds is passed to ownsAll, by having a require on the length of the _tokenIds array, as calling this function with an empty array is most likely an invalid scenario.

I cannot deploy the contract using Truffle and Ganache

I do not know if that is posible, but I could not do it.
I am working on a scanner to scan every ERC721 transaction on a blockchain, so I need to test it locally with my own blockchain.
I suspect that the reason could be that:

address presale = 0x84DFd9f27BcB2C7f621F373e5215094C486c545b; // RINKEBY TEST;

Every time that I would compile the contracts I get:

Compiling ./contracts/Etherbots/AccessControl.sol... Compiling ./contracts/Etherbots/Base.sol... Compiling ./contracts/Etherbots/ERC721.sol... Compiling ./contracts/Etherbots/EtherbotsAuction.sol... Compiling ./contracts/Etherbots/EtherbotsMigrations.sol... Compiling ./contracts/Etherbots/GenericAuction.sol... Compiling ./contracts/Etherbots/Mint.sol... Compiling ./contracts/Etherbots/NewCratePreSale.sol... Compiling ./contracts/Etherbots/PerkTree.sol... Compiling ./contracts/Etherbots/PerksRewards.sol... Error: VM Exception while processing transaction: out of gas at Object.InvalidResponse (/usr/local/lib/node_modules/truffle/build/webpack:/~/web3/lib/web3/errors.js:38:1) at /usr/local/lib/node_modules/truffle/build/webpack:/~/web3/lib/web3/requestmanager.js:86:1 at /usr/local/lib/node_modules/truffle/build/webpack:/~/truffle-migrate/index.js:225:1 at /usr/local/lib/node_modules/truffle/build/webpack:/~/truffle-provider/wrapper.js:134:1 at XMLHttpRequest.request.onreadystatechange (/usr/local/lib/node_modules/truffle/build/webpack:/~/web3/lib/web3/httpprovider.js:128:1) at XMLHttpRequestEventTarget.dispatchEvent (/usr/local/lib/node_modules/truffle/build/webpack:/~/xhr2/lib/xhr2.js:64:1) at XMLHttpRequest._setReadyState (/usr/local/lib/node_modules/truffle/build/webpack:/~/xhr2/lib/xhr2.js:354:1) at XMLHttpRequest._onHttpResponseEnd (/usr/local/lib/node_modules/truffle/build/webpack:/~/xhr2/lib/xhr2.js:509:1) at IncomingMessage.<anonymous> (/usr/local/lib/node_modules/truffle/build/webpack:/~/xhr2/lib/xhr2.js:469:1) at IncomingMessage.emit (events.js:185:15) at endReadableNT (_stream_readable.js:1101:12) at process._tickCallback (internal/process/next_tick.js:114:19)

Function defenderRevealMoves is susceptible to front-running; can be exploited to guarantee attacker win

HI I was testing this the other night in Discord (I am dmosinee in there), and was able to confirm it is a viable (and critical) exploit.

The core of the issue is around how the system allows an arbitrary number of attackers to battle a defender and then as part of the reveal process the defenders moves get sent in the clear.

The front-running attack looks like this:

1. Set up and run full node locally
2. Write a filter script in python against the node to filter incoming transactions till you find someone calling defenderRevealMoves on the etherbots contract
3. When that call shows up, pull the defenders exact moveset from the txn data (can even validate that it hashes properly to the original commit so the attacker doesn't have to waste gas if the defender entered moves wrong)
4. Fire off a contract call to attack, sending the perfect moves to go 8-0 against the defender (or more likely trying to make it look like a narrow victory if someone was working this exploit for real) with a significantly higher gas price than the defenders transaction.

The attack transaction will get processed first because of its higher gas cost, so the attacker gets added to the duel, then wins when the defenders transaction gets processed. The high gas required for defenderRevealMoves exacerbates the issue as well, because users will naturally want to use the lowest safe gas price to keep the cost reasonable, making front-running even easier and more cost effective for the attacker,

Resolving this might be tricky, because we don't want to swing an unfair advantage over to the defender with any kind of mechanism that could be used to pick and choose which battles to resolve.

One solution could be to record the datetime of each attacker that gets added to the battle along with the attackers bot and moves, and then check when the defender reveals and only process attackers who were added more than x minutes ago. This is a sloppy solution however, because if you set x too low then front-running may still be effective, and if you set it too high then legitimate attacks will frequently get ignored which wastes gas for the attackers.

A more significant gameplay change that could resolve this is to restrict a duel to be only a single attacker per defender. This kills the convenience of letting a handful of attacks build up while afk and then resolving them all at once later, but it does completely fix the front-running issue. I will be around here an bouncing on and off discord for any discussion about this issue or its solutions.

Incorrect Property Change Message in setMaxAttackers

Got a very quick one for you guys this time: in function setMaxAttackers the BattlePropertyChanged event is called with the wrong message (it says "Defender Fee" when it should say "Max Attackers"). Seems like the ol' copypasta strikes again :)

Gold and Shadow parts have the same bonus

   uint8 constant SHADOW_BONUS = 5;
   uint8 constant GOLD_BONUS = 10;

    // allow for more rarities
    // might never implement: undroppable rarities
    // 5 gold parts can be forged into a diamond
    // assumes rarity as follows: standard = 0, shadow = 1, gold = 2
    // shadow gives base 5% boost, gold 10% ...
   function getRarityBonus(uint8 move, EtherbotsBase.Part[4] parts) internal pure returns (uint8) {
        // bonus applies per part (but only if you're using the rare part in this move)
        uint8 rarity = parts[move].rarity;
        uint8 count = 0;
        if (rarity == 0) {
            // standard rarity, no bonus
            return 0;
        }
        for (uint8 i = 0; i < parts.length; i++) {
            if (parts[i].rarity == rarity) {
                count++;
            }
        }
        uint8 bonus = count * BONUS_PERCENT;
        return bonus;
   }

Currently getRarityBonus applies the same bonus to shadow and gold parts.
According to function comments shadow should give 5% boost and gold 10%.

Another problem I found is that EtherbotsBase assumes standard rarity to be 1, shadow 2 and gold 3 but this function assumes standard as 0, shadow as 1 and gold as 2.

How to fix:

   uint8 constant SHADOW_BONUS = 5;
   uint8 constant GOLD_BONUS = 10;

    // allow for more rarities
    // might never implement: undroppable rarities
    // 5 gold parts can be forged into a diamond
    // assumes rarity as follows: standard = 0, shadow = 1, gold = 2
    // shadow gives base 5% boost, gold 10% ...
   function getRarityBonus(uint8 move, EtherbotsBase.Part[4] parts) internal pure returns (uint8) {
        // bonus applies per part (but only if you're using the rare part in this move)
        uint8 rarity = parts[move].rarity;
        uint8 count = 0;
        if (rarity == 0) {
            // standard rarity, no bonus
            return 0;
        }
        for (uint8 i = 0; i < parts.length; i++) {
            if (parts[i].rarity == rarity) {
                count++;
            }
        }
        uint8 bonus = count;
        if (rarity == 1) {
            bonus *= SHADOW_BONUS;
        } else {
            bonus *= GOLD_BONUS;
        }
        return bonus;
   }

tokensOfOwner function will eventually gas out

Currently while there's a high chance that the below function will only ever get called off chain, in it's currently implementation of using a for loop over the entirety of totalSupply(), it will quickly become inefficient.

function tokensOfOwner(address _owner) external view returns(uint256[] ownerTokens) {
    uint256 tokenCount = balanceOf(_owner);

    uint256[] memory result = new uint256[](tokenCount);

    uint256 totalParts = totalSupply();
    uint256 resultIndex = 0;

    for (uint partId = 0; partId < totalParts; partId++) {
        if (partIndexToOwner[partId] == _owner) {
            result[resultIndex] = partId;
            resultIndex++;
        }
    }
    return result;
}

While this may not pose a problem for any of your future functions it could be problematic down the line for wallets / 3rd parties who may want to show all users assets and may rely on that function to get this data out.

Fix
The DharmaProtocol implementation of this uses mappings and has a good example which has been fully implemented:

https://github.com/dharmaprotocol/NonFungibleToken/blob/master/contracts/NonFungibleToken.sol

Essentially they include 2 additionally mappings to keep track of tokens

mapping(address => uint[]) internal ownerToTokensOwned;
mapping(uint => uint) internal tokenIdToOwnerArrayIndex; 

And then they do a hot swap and length reduction when a token is removed.

addExperience seems to have no validation

Hi,

I'm having a look at EtherbotsBattle.sol::addExperience. The only real validation I see is that the battles should be approved and that partIds and _exps should match in length.

The amount of exp to add doesn't seem to be validate - this method is external which should mean that any user can add any chosen amount of xp - as long as the battle was valid.

Impossible to click 'My Defenders' due to an overlay.

Reported this in Discord as Jorjik.

better use 'toast message' that appears in the top/bottom
i.e. https://material.io/guidelines/components/snackbars-toasts.html#snackbars-toasts-specs

_applyBonuses calls getPartBonus with move id, not part id

On line 629, the function getPartBonus is called with the move id:

_bonus += getPartBonus(move, parts);

However, getPartBonus's first argument should be the part id, as evidenced by the signature and the first line of getPartBonus on line 413:

function getPartBonus(uint movingPart, EtherbotsBase.Part[4] parts) internal view returns (uint8) {
        uint8 typ = _makePart(movingPart).rarity;

Right now, effectively the 4 parts with id's 0-3 are setting the rarity (#5: shouldn't this be element? Rarity is handled by getRarityBonus) that is being compared to the other parts.

Fix:

Either getPartBonus can be called with the moving part id directly:

_bonus += getPartBonus(parts[move], parts);

or to be consistent with getRarityBonus the move id could be the first paramater for getPartBonus:

function getPartBonus(uint8 move, EtherbotsBase.Part[4] parts) internal view returns (uint8) {
        uint8 typ = parts[move].rarity; // but this should be parts[move].element

getPartBonus gives bonus based on part rarity

function getPartBonus(uint movingPart, EtherbotsBase.Part[4] parts) internal view returns (uint8) {
    uint8 typ = _makePart(movingPart).rarity;
    // apply bonuses
    uint8 matching = 0;
    for (uint8 i = 0; i < parts.length; i++) {
        if (parts[i].rarity == typ) {
            matching++;
        }
    }
    // matching will never be less than 1
    uint8 bonus = (matching - 1) * BONUS_PERCENT;
    return bonus;
}

According to whitepaper this function should give out bonus when more than one part has the same element as current part.
Shouldn't variable typ be part element instead of rarity?

How to fix:

function getPartBonus(uint movingPart, EtherbotsBase.Part[4] parts) internal view returns (uint8) {
    uint8 typ = _makePart(movingPart).element;
    // apply bonuses
    uint8 matching = 0;
    for (uint8 i = 0; i < parts.length; i++) {
        if (parts[i].element == typ) {
            matching++;
        }
    }
    // matching will never be less than 1
    uint8 bonus = (matching - 1) * BONUS_PERCENT;
    return bonus;
}

var is missing in _winBattle

   function _winBattle(address attackerAddress, address defenderAddress, 
   uint8[] attackerMoves, uint8[] defenderMoves, uint[] attackerPartIds, uint[] defenderPartIds, bool isAttackerWinner
   ) internal 
   {    
       if (isAttackerWinner) {
        var (winnerExpBase, loserExpBase) = _calculateExpSplit(attackerPartIds, defenderPartIds);
        _allocateExperience(attackerAddress, attackerMoves, winnerExpBase, attackerPartIds);
        _allocateExperience(defenderAddress, defenderMoves, loserExpBase, defenderPartIds);
       } else {
        (winnerExpBase, loserExpBase) = _calculateExpSplit(defenderPartIds, attackerPartIds);   
        _allocateExperience(defenderAddress, defenderMoves, winnerExpBase, defenderPartIds);
        _allocateExperience(attackerAddress, attackerMoves, loserExpBase, attackerPartIds);
       }

        
    }

Shouldn't (winnerExpBase, loserExpBase) have var before it in else block?

Move/Perk Equality

In the _applyBonusTree function in StageBattle.sol, the perk bonus is applied twice for the DODGE move/ BODY perk and the TURRET move/TURRET perk, where it is only applied once for to the other two moves and their corresponding perks (see lines 502-503 and 542-543). The whitepaper states that there should be a fixed bonus f for each active perk, so is this intentional?

Part experience appears overstated.

My attack arm is showing 4294967293 xp. https://etherbots.io/999app/part/112 I've used it a lot, but don't think it warrants that much xp.

Update to support EIP-1102

Hi,

Paul from @MetaMask here. On November 2nd, 2018, MetaMask and other dapp browsers gave users the option to stop automatically exposing user accounts to dapps. Instead, dapps must request access to user accounts using a new asynchronous provider method: provider.enable(). Soon, this private browsing mode will be "on" by default, meaning all dapps must call provider.enable() or they will no longer work properly. We noticed that your dapp has not been updated to call this new method, so we wanted to reach out to you directly.

To make sure your dapp continues to work with MetaMask, Coinbase Wallet, Status, Mist, and other dapp browsers, the following changes should be made as soon as possible:

1. Use window.ethereum

Previously, dapps would access the MetaMask provider by using window.web3.currentProvider. While this will still work, the new, standard way to access the Ethereum provider in a Web browser is to use window.ethereum.

2. Call ethereum.enable()

The MetaMask provider won’t be populated with user accounts on page load. This means that any Web3 call or underlying RPC call that requires an account — such as sending transactions or signing messages — will fail by default. To access user accounts and initiate account-requiring RPC calls, dapps must first call ethereum.enable(). This method returns a Promise that resolves to an array of user accounts once access is approved for a given dapp. Once this approval happens, MetaMask will populate its injected provider with user accounts like normal.

For additional information, you can also check out the original blog post we put together that describes these updates in greater detail and provides example code (https://medium.com/metamask/https-medium-com-metamask-breaking-change-injecting-web3-7722797916a8). Please feel free to reach out to us directly with any questions, comments, or concerns while implementing this required change. The best way to contact us is through our official support channel at https://metamask.zendesk.com/hc/en-us/requests/new; we'll be able to quickly see and respond to messages there.

Best,

Paul

Loading times on many screens is too long, a spinner / loading indicator is required

The user might think that the screen is empty, whereas, it about 20-30 seconds content will appear.
This is a very easy fix, and potentially a significant UX improvement.

unused variable in _allocateExperience

    // allocates experience based on how many times a part was used in battle
    function _allocateExperience(address playerAddress, uint8[] moves, int32 exp, uint[] partIds) internal {
    
        int32[] memory exps = new int32[](partIds.length);
        int32 sum = 0;
        int32 each = exp / MOVE_LENGTH;
        for (uint i = 0; i < MOVE_LENGTH; i++) {
            exps[moves[i]] += each;
            sum += each;
        }
        _base.addExperience(playerAddress, partIds, exps);
    }

Variable sum is unused. Function addExperience sums experience, so it's not needed here.