Platform: Code4rena
Start Date: 09/02/2024
Pot Size: $60,500 USDC
Total HM: 17
Participants: 283
Period: 12 days
Judge:
Id: 328
League: ETH
Rank: 143/283
Findings: 9
Award: $15.02
π Selected for report: 0
π Solo Findings: 0
π Selected for report: CodeWasp
Also found by: 0x13, 0xAlix2, 0xAsen, 0xCiphky, 0xE1, 0xLogos, 0xaghas, 0xlemon, 0xlyov, 0xvj, ADM, Aamir, BARW, Bauchibred, DMoore, DanielArmstrong, Draiakoo, Fulum, GhK3Ndf, Josh4324, Kalogerone, KmanOfficial, Krace, KupiaSec, Limbooo, MidgarAudits, MrPotatoMagic, PedroZurdo, Pelz, Tendency, _eperezok, adam-idarrha, al88nsk, alexxander, alexzoid, aslanbek, blutorque, btk, c0pp3rscr3w3r, cartlex_, cats, d3e4, deadrxsezzz, denzi_, devblixt, dimulski, erosjohn, evmboi32, fnanni, grearlake, hulkvision, immeas, israeladelaja, jaydhales, jesjupyter, jnforja, juancito, klau5, korok, ktg, ladboy233, matejdb, merlinboii, novamanbg, nuthan2x, oualidpro, peanuts, petro_1912, pkqs90, pynschon, radev_sw, rouhsamad, sashik_eth, shaka, sobieski, soliditywala, stackachu, tallo, thank_you, ubl4nk, vnavascues, web3pwn, xchen1130, zhaojohnson
0.1044 USDC - $0.10
Fighter NFTs should only be transferable if they are not staked and the recipient of the transfer has not reached the maximum number of fighters allowed.
The FighterFarm
contract ensures this is the case by overriding the transferFrom(address from, address to, uint256 tokenId)
and safeTransferFrom(address from, address to, uint256 tokenId)
functions from the ERC721
contract, adding the necessary checks.
However, the inherited ERC721
contract also exposes a safeTransferFrom
function with the additional parameter data
, and this function is not overridden in the FighterFarm
contract. This makes it possible to bypass the _ableToTransfer
check by calling safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data)
.
This can cause many unwanted side effects. For one, the maximum number of fighters allowed can be exceeded. Another issue is that a fighter owner could transfer the fighter to another address to avoid losing points by making the updateBattleRecord
function revert due to an underflow error (see PoC).
Fighter owners can bypass the _ableToTransfer
check and transfer fighters to other addresses, potentially causing unwanted side effects like exceeding the maximum number of fighters allowed or avoiding losing points.
Add the following code to the contract RankedBattle.t.sol
and run forge test --mt testTransferFighterStaked
:
function testTransferFighterStaked() public { address alice = makeAddr("alice"); address alice2 = makeAddr("alice2"); _fundUserWith4kNeuronByTreasury(alice); _mintFromMergingPool(alice); uint8 tokenId = 0; // Alice stakes NRN for fighter vm.prank(alice); _rankedBattleContract.stakeNRN(3_000e18, tokenId); // Fighter wins battle and points are earned vm.prank(address(_GAME_SERVER_ADDRESS)); _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, true); // Alice transfers fighter to another address vm.prank(alice); _fighterFarmContract.safeTransferFrom(alice, alice2, 0, "0x"); assertEq(_fighterFarmContract.ownerOf(0), alice2); // Fighter loses battle and points are subtracted from fighter owner but, // since new owner does not have any points, it reverts vm.prank(address(_GAME_SERVER_ADDRESS)); vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x11)); _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true); }
Manual inspection.
function safeTransferFrom( address from, address to, - uint256 tokenId + uint256 tokenId, + bytes memory data ) public override(ERC721, IERC721) { require(_ableToTransfer(tokenId, to)); - _safeTransfer(from, to, tokenId, ""); + _safeTransfer(from, to, tokenId, data); }
safeTransferFrom(address from, address to, uint256 tokenId)
calls safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data)
, so it is only required to override this one.
ERC721
#0 - c4-pre-sort
2024-02-23T05:56:45Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-23T05:56:57Z
raymondfam marked the issue as duplicate of #739
#2 - c4-judge
2024-03-11T02:56:16Z
HickupHH3 marked the issue as satisfactory
π Selected for report: Aamir
Also found by: 0rpse, 0x11singh99, 0x13, 0xAlix2, 0xAsen, 0xBinChook, 0xCiphky, 0xE1, 0xKowalski, 0xLogos, 0xWallSecurity, 0xaghas, 0xbranded, 0xlemon, 0xlyov, 0xpoor4ever, 0xprinc, 0xvj, ADM, Aymen0909, BARW, Bauchibred, Breeje, CodeWasp, DMoore, DeFiHackLabs, Draiakoo, Fulum, GhK3Ndf, Greed, Jorgect, Josh4324, Kalogerone, KmanOfficial, Krace, Limbooo, McToady, MidgarAudits, MrPotatoMagic, PedroZurdo, Pelz, Ryonen, SovaSlava, SpicyMeatball, Tendency, Timenov, ZanyBonzy, _eperezok, al88nsk, alexxander, alexzoid, aslanbek, blutorque, btk, cartlex_, cats, csanuragjain, deadrxsezzz, denzi_, devblixt, dimulski, djxploit, erosjohn, evmboi32, fnanni, grearlake, haxatron, hulkvision, immeas, israeladelaja, jaydhales, jesjupyter, jnforja, josephdara, juancito, kiqo, klau5, korok, krikolkk, ktg, kutugu, ladboy233, lil_eth, m4ttm, matejdb, merlinboii, n0kto, ni8mare, novamanbg, nuthan2x, oualidpro, pa6kuda, peter, petro_1912, pkqs90, pynschon, sandy, sashik_eth, shaflow2, shaka, sobieski, soliditywala, solmaxis69, stackachu, tallo, thank_you, tpiliposian, ubl4nk, visualbits, vnavascues, web3pwn, xchen1130, zhaojohnson
0.0037 USDC - $0.00
Game items can be non transferable and this is handled in the safeTransferFrom
override function. However, the inherited ERC1155
contract also exposes the safeBatchTransferFrom
function, and this function is not overridden in the GameItems
contract. This means that non-transferable tokens can be transferred using the safeBatchTransferFrom
function.
Users can transfer non-transferable GameItems
tokens. The final impact for the protocol will depend on the expected use of the non-transferable tokens created.
According to the documentation and the deployment script, at launch, the protocol is meant to use the "battery" item, that will be used to initiate battles. The units of this item are refilled every 24 hours and users can buy additional units, but they are not transferable. This issue opens the possibility of users selling their unspent "battery" tokens, increasing the circulating supply, which will reduce the value of the token and the number of sales of the token by the protocol. It will also discourage some users from using the "battery" token to initiate battles, as they can sell them instead. This will in turn concentrate the use of the token in fewer users, reducing the number of battles and the overall activity of the protocol.
Add the following code to the contract GameItems.t.sol
and run forge test --mt testTransferNonTransferableToken -vv
.
function testTransferNonTransferableToken() public { _fundUserWith4kNeuronByTreasury(_ownerAddress); _gameItemsContract.mint(0, 1); _gameItemsContract.adjustTransferability(0, false); 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, ""); assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1); assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0); }
Manual inspection.
- /// @notice Safely transfers an NFT from one address to another. - /// @dev Added a check to see if the game item is transferable. - function safeTransferFrom( - address from, - address to, - uint256 tokenId, - uint256 amount, - bytes memory data - ) - public - override(ERC1155) - { - require(allGameItemAttributes[tokenId].transferable); - super.safeTransferFrom(from, to, tokenId, amount, data); - } + /// @dev Check to see if the game item is transferable. + function _beforeTokenTransfer( + address operator, + address from, + address to, + uint256[] memory ids, + uint256[] memory amounts, + bytes memory data + ) internal override(ERC1155) { + for (uint256 i = 0; i < ids.length; i++) { + require(allGameItemAttributes[ids[i]].transferable, "Non transferable token"); + } + }
Token-Transfer
#0 - c4-pre-sort
2024-02-22T04:41:42Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-22T04:41:49Z
raymondfam marked the issue as duplicate of #18
#2 - c4-pre-sort
2024-02-26T00:29:46Z
raymondfam marked the issue as duplicate of #575
#3 - c4-judge
2024-03-05T04:58:29Z
HickupHH3 marked the issue as satisfactory
π Selected for report: Abdessamed
Also found by: 0rpse, 0xAlix2, 0xAsen, 0xCiphky, 0xlemon, 0xmystery, 0xvj, ADM, Aamir, Archime, BARW, DarkTower, Draiakoo, FloatingPragma, JCN, McToady, MrPotatoMagic, OMEN, PetarTolev, Ryonen, SpicyMeatball, Tendency, VAD37, Velislav4o, VrONTg, Zac, adamn000, ahmedaghadi, alexxander, alexzoid, bhilare_, btk, cats, d3e4, denzi_, devblixt, dimulski, evmboi32, fnanni, givn, haxatron, immeas, jesjupyter, juancito, ke1caM, klau5, korok, krikolkk, matejdb, n0kto, niser93, peter, pkqs90, radin100, shaka, sl1, soliditywala, stackachu, stakog, t0x1c, vnavascues, yotov721, zhaojohnson
1.2667 USDC - $1.27
https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AAMintPass.sol#L109-L133 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L233-L262
In the documentation it is stated that "dendroids are a more exclusive class of NFTs (...) These are very rare and more difficult to obtain". And such has also been confirmed by the sponsor's comments: "Dendroids are more rare than champions".
The protocol offers two ways of minting dendroids. The first one is through the FighterFarm.claimFighters
function. This function requires sending the signature of the delegated server to check that the user has the right to mint the dendroids.
The second way is through a mint pass. For this, the user has first to claim the mint pass and then redeem. While the claim function checks that the user has the right to mint the dendroids by checking the signature received matches the mint data, the redeem function does not check again if the fighter type sent by the user matches the fighter submitted in the claim function.
This means that a user can claim a mint pass for a champion and then redeem it for a dendroid, which destroys the exclusivity factor associated with dendroids.
Users can use mint passes for champions to mint dendroids.
Add the following code to the contract FighterFarm.t.sol
and run forge test --mt testRedeemMintPassChangingFighterData
.
function testRedeemMintPassChangingFighterData() public { // Delegated server signs mintpass for 1 champion to the user uint8[2] memory numToMint = [1, 0]; // π index 0 for champion bytes memory signature = abi.encodePacked( hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c" ); string[] memory _tokenURIs = new string[](1); _tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m"; // Mint NFT from mintpass contract _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs); 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); // Redeem mint pass to mint 1 dendroid _mintpassIdsToBurn[0] = 1; _mintPassDNAs[0] = "dna"; _fighterTypes[0] = 1; // π 1 = dendroid _neuralNetHashes[0] = "neuralnethash"; _modelTypes[0] = "original"; _iconsTypes[0] = 1; _mintPassContract.approve(address(_fighterFarmContract), 1); _fighterFarmContract.redeemMintPass( _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes ); (,,,,,,,, bool dendroidBool) = _fighterFarmContract.fighters(0); assertEq(dendroidBool, true); }
Manual inspection.
Store the fighter type associated with the mint pass in the AAMintPass
contract and validate that the fighter type passed by the user in FighterFarm.redeemMintPass
matches the value stored in the AAMintPass
contract.
Invalid Validation
#0 - c4-pre-sort
2024-02-22T08:20:32Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-22T08:20:39Z
raymondfam marked the issue as duplicate of #33
#2 - c4-pre-sort
2024-02-26T00:54:10Z
raymondfam marked the issue as duplicate of #1626
#3 - c4-judge
2024-03-05T10:56:27Z
HickupHH3 changed the severity to 3 (High Risk)
#4 - c4-judge
2024-03-06T03:36:28Z
HickupHH3 marked the issue as satisfactory
π Selected for report: klau5
Also found by: 0xAleko, 0xAlix2, 0xAsen, 0xCiphky, 0xKowalski, 0xlemon, 0xvj, 14si2o_Flint, Aamir, AlexCzm, Aymen0909, BARW, Blank_Space, DanielArmstrong, Davide, Draiakoo, Giorgio, McToady, MrPotatoMagic, PoeAudits, Ryonen, Silvermist, SpicyMeatball, Tychai0s, VAD37, Varun_05, alexxander, alexzoid, aslanbek, blutorque, btk, cats, d3e4, denzi_, evmboi32, fnanni, givn, grearlake, haxatron, jesjupyter, juancito, ke1caM, ktg, lanrebayode77, linmiaomiao, matejdb, merlinboii, n0kto, novamanbg, nuthan2x, petro_1912, pynschon, radin100, sashik_eth, shaka, sl1, soliditywala, solmaxis69, t0x1c, ubl4nk, vnavascues, xchen1130, yotov721, zhaojohnson
1.1225 USDC - $1.12
FighterFarm.reRoll
allows the owner of a fighter to regenerate its element, weight, and physical attributes. The number of rerolls per fighter is limited to a certain number of times depending on the fighter type.
The problem is that the fighter type is passed as an argument and the function does not check that this value matches the actual fighter type for the given fighter. This has the following implications:
fighterType
parameter as 0 (champion).Fighter owners can reroll their fighters more times than they should be able to and can simulate the outcome of the reroll with the two fighter types and choose the one that gives the best result.
Manual inspection.
File: FighterFarm.sol - function reRoll(uint8 tokenId, uint8 fighterType) public { + function reRoll(uint8 tokenId) public { require(msg.sender == ownerOf(tokenId)); + uint8 fighterType = fighters[tokenId].dendroidBool ? 1 : 0; require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
Invalid Validation
#0 - c4-pre-sort
2024-02-22T02:46:12Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-22T02:46:18Z
raymondfam marked the issue as duplicate of #306
#2 - c4-judge
2024-03-05T04:37:32Z
HickupHH3 marked the issue as satisfactory
#3 - c4-judge
2024-03-19T09:05:07Z
HickupHH3 changed the severity to 3 (High Risk)
π Selected for report: t0x1c
Also found by: 0rpse, 0xAadi, 0xBinChook, 0xCiphky, 0xDetermination, 14si2o_Flint, AC000123, Aamir, Abdessamed, Blank_Space, CodeWasp, DanielArmstrong, DarkTower, Draiakoo, Honour, Kalogerone, Krace, McToady, Merulez99, MidgarAudits, MrPotatoMagic, PedroZurdo, Silvermist, Tychai0s, VAD37, Velislav4o, VrONTg, WoolCentaur, YouCrossTheLineAlfie, ZanyBonzy, alexxander, aslanbek, btk, csanuragjain, d3e4, dimulski, djxploit, erosjohn, evmboi32, fnanni, forgebyola, forkforkdog, handsomegiraffe, immeas, israeladelaja, juancito, ktg, n0kto, neocrao, ni8mare, okolicodes, peanuts, petro_1912, shaflow2, shaka, swizz, ubermensch, ubl4nk, yotov721
2.0593 USDC - $2.06
When a fighter has NRN staked and loses a battle, there are two possible scenarios:
If the user has accumulated points from previous battles, the points are decreased.
If the user has no points, some of the staked NRN is set at risk.
Given that the requirement for the first scenario is just having accumulated points higher than 0, in case the user has very few points but a lot of NRN staked, the user can engage in battles with barely any downside risk, but a lot of upside potential.
A possible strategy would be to stake a very low amount of NRN and engage in battles with fighters that can be easily defeated and, after having accumulated at least one point, increase the stake to a very high amount and then engage in battles knowing that if the battle is lost, the user will only lose a few points, but if the battle is won, the user will win a lot of them.
Fighter owners can engage in battles with almost no downside risk, but a lot of upside potential.
Manual inspection.
Adjust the _addResultPoints
function to add stake at risk if the fighter does not have enough points to cover the curStakeAtRisk
amount.
Other
#0 - c4-pre-sort
2024-02-22T17:25:18Z
raymondfam marked the issue as insufficient quality report
#1 - c4-pre-sort
2024-02-22T17:25:25Z
raymondfam marked the issue as duplicate of #38
#2 - c4-judge
2024-03-07T02:58:22Z
HickupHH3 changed the severity to 3 (High Risk)
#3 - c4-judge
2024-03-07T03:39:36Z
HickupHH3 marked the issue as satisfactory
π Selected for report: haxatron
Also found by: 0xAlix2, 0xCiphky, 0xStriker, 0xaghas, 0xbranded, 0xlamide, 0xmystery, 0xvj, 14si2o_Flint, Aamir, AgileJune, Aymen0909, DanielArmstrong, DarkTower, Draiakoo, EagleSecurity, Giorgio, Krace, KupiaSec, MidgarAudits, MrPotatoMagic, PoeAudits, Ryonen, SpicyMeatball, Topmark, Tychai0s, VAD37, Varun_05, VrONTg, WoolCentaur, _eperezok, alexxander, alexzoid, aslanbek, blutorque, btk, cartlex_, d3e4, devblixt, evmboi32, fnanni, immeas, jesjupyter, juancito, ke1caM, klau5, ktg, lil_eth, merlinboii, nuthan2x, peter, petro_1912, pkqs90, pynschon, radin100, sandy, sashik_eth, shaka, sl1, soliditywala, t0x1c, ubl4nk, visualbits, vnavascues
1.1225 USDC - $1.12
https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L85 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L470
FighterFarm
contract allows minting fighters through three functions: claimFighters
, redeemMintPass
and mintFromMergingPool
.
Both claimFighters
and redeemMintPass
pass the value 100 to the customAttributes
parameter of the internal function _createFighter
.
This makes the code execute the _createFighterBase
function.
In the _createFighterBase
function, the elements
variable is calculated as the module of the dna
and the number of elements for the current generation of the fighterType
.
The problem is that the numElements
is only set in constructor for generation 0, but cannot be set for other generations. So after the generation is increased for a fighter type via the incrementGeneration
function, numElements[generation[fighterType]]
will be 0 and the execution will revert due to a modulo by zero error.
This means that fighters of generation 1 or higher will only be mintable from the merging pool. Note that fighters minted from the merging pool can only be of type champion, so dendroids will not be mintable at all.
Another thing that is worth mentioning is that the numElements
is associated with a generation, but not with a fighter type. So the protocol lacks the flexibility to have different values for different fighter types in the same generation.
For fighter generations higher than 0 champions will only be mintable from the merging pool and dendroids will not be mintable at all.
Add the following code to the contract FighterFarm.t.sol
and run forge test --mt testFighterCreationBroken
.
function testFighterCreationBroken() public { uint8[2] memory numToMint = [1, 0]; bytes memory claimSignature = abi.encodePacked( hex"407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c" ); string[] memory claimModelHashes = new string[](1); claimModelHashes[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m"; string[] memory claimModelTypes = new string[](1); claimModelTypes[0] = "original"; // Champion generation increases _fighterFarmContract.incrementGeneration(0); // Claim fighter vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x12)); _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes); }
Manual inspection.
Add an admin function to set the values of numElements
.
Also, consider having different values depending on the fighter type.
Error
#0 - c4-pre-sort
2024-02-22T19:10:53Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-22T19:11:02Z
raymondfam marked the issue as duplicate of #45
#2 - c4-judge
2024-03-08T03:20:05Z
HickupHH3 marked the issue as satisfactory
π Selected for report: klau5
Also found by: 0rpse, 0xBinChook, 0xDetermination, 0xGreyWolf, 0xLogos, 0xWallSecurity, 0xaghas, 0xgrbr, 0xkaju, 0xlyov, AlexCzm, BARW, Blank_Space, BoRonGod, Daniel526, DanielArmstrong, Draiakoo, FloatingPragma, Giorgio, Greed, Jorgect, Matue, McToady, MidgarAudits, Nyxaris, PUSH0, PedroZurdo, Pelz, PoeAudits, Silvermist, SpicyMeatball, Tekken, Tricko, Tumelo_Crypto, VAD37, WoolCentaur, Zac, alexzoid, andywer, aslanbek, bgsmallerbear, cats, d3e4, desaperh, dimulski, dutra, erosjohn, evmboi32, favelanky, fnanni, forkforkdog, gesha17, givn, grearlake, haxatron, honey-k12, iamandreiski, immeas, juancito, kaveyjoe, ke1caM, kiqo, klau5, korok, lil_eth, lsaudit, n0kto, ni8mare, niser93, pa6kuda, peanuts, peter, shaka, sl1, soliditywala, solmaxis69, t0x1c, tallo, thank_you, tpiliposian, visualbits, vnavascues, web3pwn, yotov721
0.0352 USDC - $0.04
https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L470-L472 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L107 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L254
The dna variable is used to generate certain properties of the fighter, such as element, weight and physical attributes.
While the minter cannot use a specific dna for the generation of the fighter properties, he has some control over its value, especially in the redeemMintPass
function, where the dna is generated from the keccak256
of the value passed by the user.
This implies that the minter can simulate the generation of a fighter with different mintPassDnas
values until he finds a value that generates a fighter with the desired properties.
The minter has some control over the properties of the fighter, which could be used to generate fighters with the desired properties, some of which determine the outcome of battles.
Manual inspection.
Use Chainlink VRF as a source of randomness.
Other
#0 - c4-pre-sort
2024-02-24T02:03:11Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-24T02:03:23Z
raymondfam marked the issue as duplicate of #53
#2 - c4-judge
2024-03-06T03:49:26Z
HickupHH3 changed the severity to 3 (High Risk)
#3 - c4-judge
2024-03-06T03:54:00Z
HickupHH3 marked the issue as satisfactory
#4 - c4-judge
2024-03-15T02:10:54Z
HickupHH3 changed the severity to 2 (Med Risk)
#5 - c4-judge
2024-03-22T04:23:07Z
HickupHH3 marked the issue as duplicate of #376
π Selected for report: klau5
Also found by: 0xAlix2, 0xCiphky, 0xDetermination, 0xG0P1, 0xMosh, 0xabhay, 14si2o_Flint, AlexCzm, Aymen0909, CodeWasp, DanielArmstrong, FloatingPragma, Giorgio, JCN, Jorgect, Kalogerone, KmanOfficial, Kow, KupiaSec, McToady, SpicyMeatball, VAD37, WoolCentaur, ZanyBonzy, alexxander, alexzoid, almurhasan, blutorque, csanuragjain, denzi_, dipp, djxploit, evmboi32, handsomegiraffe, haxatron, immeas, jesjupyter, ke1caM, klau5, lanrebayode77, lil_eth, merlin, merlinboii, nuthan2x, peanuts, shaflow2, shaka, sl1, solmaxis69, stakog, swizz, t0x1c, tallo, ubermensch, vnavascues, yotov721
0.5044 USDC - $0.50
https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L333 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L461 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/StakeAtRisk.sol#L104
RankedBattle.updateBattleRecord
is called by the game server address to update the battle record of a fighter.
First the function gets the current owner of the fighter.
If the fighter has amount staked or stake at risk, it calls the internal function _addResultPoints
.
This function handles different scenarios, two of which result in the stake at risk being updated in the StakeAtRisk
contract.
In the case of the battle being lost and the fighter does not have any points the stake at risk is increased.
In the case of the battle is won and the fighter has stake at risk, part or all of the stake at risk is reclaimed.
Now let's take a look at how the StakeAtRisk
contract handles these changes in the stake at risk.
updateAtRiskRecords
updates the mappings stakeAtRisk
, totalStakeAtRisk
, and amountLost
increasing their values by the amount of NRN received as parameter. For its part, reclaimNRN
decreases these same mappings by the amount of NRN received as parameter.
The important thing to note is that the value in the amountLost
mapping is updated for the fighter owner. Given that the owner of the fighter can change between battles, this can lead to accounting errors that can in turn cause the transaction to revert (see PoC below).
DoS for RankedBattle.updateBattleRecord
, which implies the fighter owner not being able to recover the stake at risk, the battle record and the total number of battles not being updated, and, in case the fighter owner was the initiator of the battle, the voltage not being subtracted from him.
Add the following code to the contract RankedBattle.t.sol
and run forge test --mt testAmountLostUnderflows
.
function testAmountLostUnderflows() public { address alice = makeAddr("alice"); address bob = makeAddr("bob"); _mintFromMergingPool(alice); uint8 tokenId = 0; _fundUserWith4kNeuronByTreasury(alice); // Alice stakes NRN for fighter vm.prank(alice); _rankedBattleContract.stakeNRN(3_000e18, tokenId); // Fighter loses battle (stake at risk is registered) vm.prank(address(_GAME_SERVER_ADDRESS)); _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true); // Alice sells fighter to Bob vm.startPrank(alice); _rankedBattleContract.unstakeNRN(3_000e18, tokenId); _fighterFarmContract.transferFrom(alice, bob, tokenId); vm.stopPrank(); // Fighter wins battle (stake at risk is reclaimed) vm.prank(address(_GAME_SERVER_ADDRESS)); vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x11)); _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, true); }
Manual inspection.
Given that StakeAtRisk.amountLost
is not used in any other part of the protocol and is probably just an informational variable, the most straightforward fix would be to remove it from the contract.
If it is considered necessary to keep this variable, it should be updated whenever an NFT is transferred, adjusting the values for the previous and new owner.
Under/Overflow
#0 - c4-pre-sort
2024-02-23T20:04:26Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-23T20:04:37Z
raymondfam marked the issue as duplicate of #1641
#2 - c4-judge
2024-03-12T03:38:07Z
HickupHH3 marked the issue as satisfactory
#3 - c4-judge
2024-03-12T04:01:25Z
HickupHH3 changed the severity to 3 (High Risk)
#4 - c4-judge
2024-03-12T04:03:31Z
HickupHH3 changed the severity to 2 (Med Risk)
#5 - c4-judge
2024-03-13T09:52:47Z
HickupHH3 marked the issue as partial-50
π Selected for report: givn
Also found by: 0x11singh99, 0xAkira, 0xBinChook, 0xDetermination, 0xMosh, 0xStriker, 0xmystery, 14si2o_Flint, 7ashraf, Aamir, AlexCzm, BARW, Bauchibred, BenasVol, BigVeezus, Blank_Space, Bube, DarkTower, DeFiHackLabs, EagleSecurity, KmanOfficial, Krace, McToady, MrPotatoMagic, PetarTolev, Rolezn, SHA_256, SpicyMeatball, Tekken, Timenov, ZanyBonzy, agadzhalov, alexzoid, boredpukar, btk, cartlex_, dimulski, forkforkdog, haxatron, immeas, jesjupyter, juancito, kartik_giri_47538, klau5, lsaudit, merlinboii, nuthan2x, offside0011, oualidpro, peter, radev_sw, rekxor, rspadi, shaflow2, shaka, swizz, vnavascues, yotov721, yovchev_yoan
8.8123 USDC - $8.81
ID | Title |
---|---|
[L-01] | MergingPool:getFighterPoints reverts when called with maxId greater than 1 |
[L-02] | The value of RankedBattle.totalBattles will be doubled |
[L-03] | Signature verification in FighterFarm.claimFighters will always pass if _delegatedAddress is set to address zero |
[L-04] | Signature in FighterFarm.claimFighters can be replayed in other chains |
MergingPool:getFighterPoints
reverts when called with maxId
greater than 1The size of the array returned by getFighterPoints
is set to 1, so this function can only be used to get the points of fighter with id 0. This makes it not possible to use this function for on-chain integrations with the protocol.
Additionally, name and description of the parameter maxId
is misleading, as it is not the maximum id of the fighters, but the number of fighters to return points for. E.g. maxId = 1 returns up to fighter with id 0.
Consider applying the following changes:
File: MergingPool.sol function getFighterPoints(uint256 maxId) public view returns(uint256[] memory) { - uint256[] memory points = new uint256[](1); + uint256[] memory points = new uint256[](maxId + 1); - for (uint256 i = 0; i < maxId; i++) { + for (uint256 i = 0; i <= maxId; i++) { points[i] = fighterPoints[i]; } return points; }
RankedBattle.totalBattles
will be doubledEach time the game server address calls updateBattleRecord
, the value of totalBattles
is incremented by 1. As this method should be called twice for battle (once for each fighter), the value of totalBattles
will be doubled.
Consider sending the data for both fighters in a single call to updateBattleRecord
.
FighterFarm.claimFighters
will always pass if _delegatedAddress
is set to address zeroIn the case the FighterFarm
is deployed setting the _delegatedAddress
to address zero, the signature verification in the claimFighters
function will always pass. Given that this misconfiguration could not be evident in the early stages of the protocol, but lead to a high-impact issue (free minting of fighters), it is recommended to add a check in the constructor to prevent the value of _delegatedAddress
from being set to address zero.
FighterFarm.claimFighters
can be replayed in other chainsIf the protocol decides to deploy the FighterFarm contract in other chains and use the sale delegated address to sign the messages for claiming fighters, the same signature can be replayed in different chains to claim fighters in all of them.
Consider adding a chain id to the message to prevent replay attacks.
ID | Title | Instances |
---|---|---|
[N-01] | Inconsistent use of attributes array size | 1 |
[N-02] | Max NRN supply cannot be reached | 1 |
[N-03] | Unnecessary code | 2 |
[N-04] | Incorrect comments | 8 |
attributes
array sizeIn the AiArenaHelper contract the size of the attributes
array is used in different places to check for valid input data and to iterate over the size of the array. In some places it is used as attributes.length
and in other places the value is hardcoded as 6
.
Consider using a constant to store the size of the array for consistency and gas optimization.
The mint function in the Neuron contract has a check to prevent minting more than the max supply, but the condition is incorrect. The condition should be totalSupply() + amount <= MAX_SUPPLY
instead of totalSupply() + amount < MAX_SUPPLY
.
File: AiArenaHelper.sol 41 constructor(uint8[][] memory probabilities) { 42 _ownerAddress = msg.sender; 43 44 // Initialize the probabilities for each attribute 45 addAttributeProbabilities(0, probabilities); π 46 47 uint256 attributesLength = attributes.length; 48 for (uint8 i = 0; i < attributesLength; i++) { 49 attributeProbabilities[0][attributes[i]] = probabilities[i]; 50 attributeToDnaDivisor[attributes[i]] = defaultAttributeDivisor[i]; π Set twice 51 } 52 } (...) 68 function addAttributeDivisor(uint8[] memory attributeDivisors) external { 69 require(msg.sender == _ownerAddress); 70 require(attributeDivisors.length == attributes.length); 71 72 uint256 attributesLength = attributes.length; 73 for (uint8 i = 0; i < attributesLength; i++) { 74 attributeToDnaDivisor[attributes[i]] = attributeDivisors[i]; π 75 } 76 }
File: FighterFarm.sol 443 /// @notice Hook that is called before a token transfer. 444 /// @param from The address transferring the token. 445 /// @param to The address receiving the token. 446 /// @param tokenId The ID of the NFT being transferred. 447 function _beforeTokenTransfer(address from, address to, uint256 tokenId) 448 internal 449 override(ERC721, ERC721Enumerable) 450 { 451 super._beforeTokenTransfer(from, to, tokenId); π Just calls the default implementation 452 }
File: MergingPool.sol 136 /// @param modelURIs The array of model URIs corresponding to each round and winner address. π 137 /// @param modelTypes The array of model types corresponding to each round and winner address. 138 /// @param customAttributes Array with [element, weight] of the newly created fighter. 139 function claimRewards( 140 string[] calldata modelURIs, π (...) 154 _fighterFarmInstance.mintFromMergingPool( 155 msg.sender, 156 modelURIs[claimIndex], π This value is described as `modelHash The hash of the ML model associated with the fighter` in the FighterFarm contract
File: MergingPool.sol 50 /// @notice Mapping of address to fighter points. π It should say "Mapping of **fighter ID** to points." 51 mapping(uint256 => uint256) public fighterPoints;
File: FighterFarm.sol 50 /// The address that has owner privileges (initially the contract deployer). π Not contract deployer, but value set in constructor 51 address _ownerAddress;
File: GameItems.sol 60 /// The address that has owner privileges (initially the contract deployer). π Not contract deployer, but value set in constructor 61 address _ownerAddress;
File: MergingPool.sol 34 /// The address that has owner privileges (initially the contract deployer). π Not contract deployer, but value set in constructor 35 address _ownerAddress;
File: Neuron.sol 48 /// The address that has owner privileges (initially the contract deployer). π Not contract deployer, but value set in constructor 49 address _ownerAddress;
File: RankedBattle.sol 71 /// The address that has owner privileges (initially the contract deployer). π Not contract deployer, but value set in constructor 72 address _ownerAddress;
File: VoltageManager.sol 22 /// The address that has owner privileges (initially the contract deployer). π Not contract deployer, but value set in constructor 23 address _ownerAddress;
#0 - raymondfam
2024-02-26T07:02:00Z
L1 to #48
#1 - c4-pre-sort
2024-02-26T07:02:03Z
raymondfam marked the issue as sufficient quality report
#2 - c4-judge
2024-03-15T06:42:57Z
HickupHH3 marked the issue as grade-c
#3 - HickupHH3
2024-03-21T03:17:32Z
#1908: R L-01: L L-02: L L-03: L L-04: R
#4 - c4-judge
2024-03-21T03:18:42Z
HickupHH3 marked the issue as grade-b