etherbots / etherbots-bug-bounty Goto Github PK
View Code? Open in Web Editor NEWCode implemented on Rinkeby testnet, along with a wiki guide for the Bug Bounty.
Code implemented on Rinkeby testnet, along with a wiki guide for the Bug Bounty.
// 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.
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.
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.
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?
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
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.
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.
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:
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.
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.
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.
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;
}
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;
}
Impossible to see artwork in the top/center of the screen, because it overlaps.
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.
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
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));
}
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));
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
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?
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)
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.
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
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.
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
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 :)
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:
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
.
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
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.