forge install
forge test
The struct of the Mon
is
struct Mon {
uint8 water;
uint8 air;
uint8 fire;
uint8 speed;
}
so the correct order when creating a new Mon
should be
-newMon = Mon(fire, water, air, speed);
+newMon = Mon(water, air, fire, speed);
This is not a huge issue just because all the properties are generated with the same max not inclusive value (10). Still is an error that should be fixed.
randomGen(uint256 i)
is a function that generates a number from 0 to to i-1
. As the name state it's a pseudo random generator, and it's possible to predict which would be the random number generated even if the nonce
used is private.
A contract could run the same code used by the function and if the result is not what they expect (for example a Mon that is not maxed out) it could just revert the transaction before finalizing.
After a swap the forSale
mapping should be resetted to false
otherwise the swapped Mon will remain forever in a "sellable" state even if the new owner does not want to sell it anymore.
Scenario:
- Bob want to sell
Mon1
so it callgame.putUpForSale(mon1);
- Alice want to swap
Mon2
forMon1
because it's much more powerful. Alice callgame.swap(address(Bob), mon2, mon1)
and the swap happen - Paul see the transaction and is able to swap again the Mon1 that Alice just purchased because the Mon is still "sellable". Paul call
game.swap(address(Alice), mon3, mon1)
and swap the new Alice's mon even if the not intended to sell it
Alice has a medium powerful Mon1 that want to sell, let's say a Mon(5,5,5,5)
Bob see that Alice has put Mon1
for sale, and he decides to swap it with his Mon2
that is the worst Mon available Mon(1,1,1,1)
.
Alice cannot decide to accept the transaction and Bob gain, without losing anything, a better Mon.
function indexInDeck(address _owner, uint256 _monId) internal view returns(uint256 idx) {
for (uint256 i; i < DECK_SIZE; i++) {
if (decks[_owner][i] == _monId) {
idx = i;
}
}
}
If the if
statement is never true, the function will return the default value for the idx
parameter that is 0
. So, even if no Mon with the ID _monId
is found in the _owner
's deck, the function will return 0
like if the not present Mon was present in the first (0 index) position.
The function should return inside the if
and revert
if it arrives at the end of the for
without finding a match.
swap
is vulnerable to reentrancy attack because of _safeTransfer
and miss of following Checks-Effects-Interactions Pattern best practice or reentrancy guard
_safeTransfer
is a function that internally call a "callback" onERC721Received
if the to
address is a contract
. This is needed to be sure that the receiving address is able to handle ERC721 tokens (NFT).
The problem is that the function does not:
- implement Reentrancy Guard
- update the contract's state variables after the external call
Scenario the to
is a Contract that implement the onERC721Received
callback
- Alice has mon1, mon2, mon3 in the deck
- Bob deploy an
attacker
contract that implementonERC721Received
andjoin
the game with the contract. The contract has mon4, mon5, mon6 in the deck - Alice put for sale mon1
- Bob call
attacker.game.swap(alice, mon4, mon1)
when _safeTransfer(_to, swapper, _monId2, "");
is executed, the Game
call attacker.onERC721Received
callback. At this point the decks
are not yet updated (they are updated only after the second _safeTransfer
.
This mean that inside onERC721Received
mon1
is already owned by Alice butdeck[alice][0]
= mon1deck[attacker][0]
= mon4
At this point the attacker is able for example to call game.fight()
where the game would fight with deck[attacker][0]
that still point to mon4
even if it has already been transferred to Alice
.
So when the attacker
will be defeated by the unbeatable flagholder
the game will burn mon4
that is currently owned (because it has been transferred) by Alice
and not by the attacker
. Note that _burn
is a low level instruction that is not checking who's the owner of the tokenId.
After the fight
end all the attacker mon will be burned and new mon will be replaced to the burned one.
After returning from the onERC721Received
callback the state of the attacker
deck is like this
- deck[0] = mon7
- deck[1] = mon8
- deck[2] = mon9
and the following instructions will be executed
// update the decks
uint256 idx1 = indexInDeck(swapper, _monId1);
uint256 idx2 = indexInDeck(_to, _monId2);
decks[swapper][idx1] = _monId2;
decks[_to][idx2] = _monId1;
indexInDeck(swapper, mon4);
will return 0 because it will try to find a token with ID 4 inside the deck but the user deck has been replaced because the user has lost thefight
and now it has inside mon7, mon8, mon9. Because of the problem we have discussed before, instead of reverting when it does not find a token that should be in the deck, it will return the default value ofidx
return variable that is0
indexInDeck(_to, mon1);
will return 0 becausedeck[0] = mon1
decks[attacker][0] = mon1
(mon1 is still alive)decks[alice][0] = mon4
(mon4 has been burned becauseattacker
has lost thefight
)
So at the end of the cycle the attacker
ends with 4 Mon instead of 3 because it has 3 new Mon (minted by fight
to replenish the deck) + the one that has been swapped from Alice.
It will be a mix of all the problem we have seen, in particular we are going to use both the reentrancy problem in swap
and the indexInDeck
not reverting when the Mon is not present in the deck.
We could also leverage the fact that forSell
is not updated, but it's not relevant because we will use different addresses to exploit it. In a normal scenario, we could use the forSell
exploit to gain access to other user's Mon that have been put for sale at least one time.
What we do is:
- we create an
Exploiter
smart contract that is able to interact with thegame
. The smart contract is inheriting fromIERC721Receiver
and implementing theonERC721Received
callback. This callback will be called automatically by thegame
during aswap
operation.
Inside the callback, we are just going to call game.fight();
. If the callback has been called because of exploiter.swap(alice, monOfAttackInPos0, monOfAliceInPos0)
it will make the attacker attack with monOfAttackInPos0
in position 0 so the game
will burn the Mon that after the callback will be transferred to Alice.
After the end of swap
the Exploiter, contract will own 4 different Mons (3 new Mons after fight
+ 1 Mon from Alice)
- Use two different accounts to join the game and fill their decks. After that, put all the Mons for sale
- Make the
exploiter
join the game and fill its deck and put all the Mons for sale. - Execute
exploiter.swap(address(attacker2), 9, 3);
to leverage the reentrancy exploit. At the end of eachswap
theexploiter
contract will have the Mon balance increased by 1 (as explained before) - Keep executing the exploit until you have at least 7 mon before starting the fight. This is important because after the fight the game will check if the attacker (you) have more tokens compared to the flag holder. If so you (attacker) will become the flag holde!. As soon as
attacker2
has finished the swappable Mons (that are burned each time the swap happen because of thefight
) start usingattacker3
Mons. - Profit and enjoy being the Flag Holder! (well not you but your exploit contract :D