Submitted by CodeWasp, also found by adam-idarrha, d3e4, vnavascues, c0pp3rscr3w3r, shaka, GhK3Ndf, alexxander, Fulum, thank_you, denzi_, Limbooo, korok, evmboi32, juancito, grearlake, ktg, radev_sw, rouhsamad, KmanOfficial, MidgarAudits, KupiaSec, 0xaghas, Bauchibred, jaydhales, immeas, MrPotatoMagic, soliditywala, sashik_eth, 0xLogos, Tendency, 0x13, PedroZurdo, al88nsk, ladboy233 (1, 2), merlinboii, DanielArmstrong, hulkvision, israeladelaja, Draiakoo, ADM, petro_1912, nuthan2x, 0xCiphky, peanuts, cats, 0xAsen, stackachu, deadrxsezzz, Krace, jnforja, 0xvj, _eperezok, pkqs90, Josh4324, web3pwn, Aamir, btk, pynschon (1, 2), 0xE1, ubl4nk, fnanni, devblixt, cartlex_, erosjohn, BARW, dimulski, zhaojohnson, blutorque, Kalogerone, DMoore, jesjupyter, 0xlemon, klau5, 0xAlix2, alexzoid, novamanbg, sobieski, xchen1130, matejdb, tallo, Pelz, oualidpro, aslanbek, and 0xlyov
FighterFarm contract implements restrictions on the transferability of fighters in functions transferFrom() and safeTransferFrom(), via the call to function _ableToTransfer(). Unfortunately this approach doesnt cover all possible ways to transfer a fighter:  The FighterFarm contract inherits from OpenZeppelins ERC721 contract, which includes the public function safeTransferFrom(, data), i.e. the same as safeTransferFrom() but with the additional data parameter. This inherited function becomes available in the GameItems contract, and calling it allows us to circumvent the transferability restriction. As a result, a player will be able to transfer any of their fighters, irrespective of whether they are locked or not. Violation of such a basic system invariant leads to various kinds of impacts, including:
Both of the last two impacts include the inability of the game server to commit certain transactions, so we illustrate both of the last two with PoCs, thus illustrating the first one as well.
If a fighter wins a battle, points are added to accumulatedPointsPerAddress mapping. When a fighter loses a battle, the reverse happens: points are subtracted. If a fighter is transferred after it wins the battle to another address, accumulatedPointsPerAddress for the new address is empty, and thus the points cant be subtracted: the game server transaction will be reverted. By transferring the fighter to a new address after each battle, the fighter becomes unstoppable, as its accumulated points will only grow, and will never decrease.
If a fighter loses a battle, funds are transferred from the amount at stake, to the stake-at risk, which is reflected in the amountLost mapping of StakeAtRisk contract. If the fighter with stake-at-risk is transferred to another player, the invariant that amountLost reflects the lost amount per address is violated: after the transfer the second player has more stake-at-risk than before. A particular way to exploit this violation is demonstrated below: the transferred fighter may win a battle, which leads to reducing amountLost by the corresponding amount. Upon subsequent wins of the second player own fighters, this operation will underflow, leading to the game server unable to commit transactions, and the player unable to exit the losing zone. This effectively makes a fighter with the stake-at-risk a poison pill.
Remove the incomplete checks in the inherited functions transferFrom() and safeTransferFrom() of FighterFarm contract, and instead to enforce the transferability restriction via the _beforeTokenTransfer() hook, which applies equally to all token transfers, as illustrated below.
brandinho (AI Arena) confirmed
AI Arena mitigated:
Status: Mitigation confirmed. Full details in reports from niser93, d3e4, and fnanni.
