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: 104/283
Findings: 11
Award: $59.67
๐ Selected for report: 0
๐ Solo Findings: 0
๐ 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
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol#L291 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC1155/ERC1155.sol#L120
Users can transfer non-transferable game items with safeBatchTransfer
Creator of the game item token can set transferable
option to false and bind token to one account restricting any transfers.
function createGameItem( string memory name_, string memory tokenURI, bool finiteSupply, bool transferable, uint256 itemsRemaining, uint256 itemPrice, uint16 dailyAllowance ) public { require(isAdmin[msg.sender]); allGameItemAttributes.push( GameItemAttributes( name_, finiteSupply, >> transferable, itemsRemaining, itemPrice, dailyAllowance ) );
This restriction can be bypassed since only safeTransferFrom
has a transferability check while safeBatchTransferFrom
doesn't.
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol#L301
Coded POC for GameItems.t.sol
function testTransferSoulbound() public { _fundUserWith4kNeuronByTreasury(_ownerAddress); _gameItemsContract.createGameItem("Soulbound", "https://ipfs.io/ipfs/", true, false, 10_000, 1 * 10 ** 18, 10); _gameItemsContract.mint(1, 1); vm.expectRevert(); _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 1, 1, ""); uint256[] memory ids = new uint256[](1); uint256[] memory values = new uint256[](1); ids[0] = 1; values[0] = 1; _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, values, ""); }
Foundry
Restrict safeBatchTransferFrom
as well
Token-Transfer
#0 - c4-pre-sort
2024-02-22T03:35:23Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-22T03:35:29Z
raymondfam marked the issue as duplicate of #18
#2 - c4-pre-sort
2024-02-26T00:27:25Z
raymondfam marked the issue as duplicate of #575
#3 - c4-judge
2024-03-05T04:47:39Z
HickupHH3 changed the severity to 3 (High Risk)
#4 - c4-judge
2024-03-05T04:51:21Z
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/main/src/FighterFarm.sol#L235-L236
User can mint rare type of fighters like dendroids and icons, which shouldn't be mintable.
According to docs https://docs.aiarena.io/gaming-competition/nft-makeup
๐บ Dendroids - Dendroids are a more exclusive class of NFTs. They have the ability to incorporate other metaverse assets (e.g. NFTs from other projects) into the Arena! These are very rare and more difficult to obtain. Youโll have to follow the story to find out how to get access to one of these!
Icons are special cosmetic items https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L83
function createPhysicalAttributes( uint256 dna, uint8 generation, uint8 iconsType, bool dendroidBool ) external view returns (FighterOps.FighterPhysicalAttributes memory) { >> if (dendroidBool) { return FighterOps.FighterPhysicalAttributes(99, 99, 99, 99, 99, 99); } else { uint256[] memory finalAttributeProbabilityIndexes = new uint[](attributes.length); uint256 attributesLength = attributes.length; for (uint8 i = 0; i < attributesLength; i++) { if ( >> i == 0 && iconsType == 2 || // Custom icons head (beta helmet) >> i == 1 && iconsType > 0 || // Custom icons eyes (red diamond) >> i == 4 && iconsType == 3 // Custom icons hands (bowling ball) ) { finalAttributeProbabilityIndexes[i] = 50;
Both icons and dendroids are free to mint with redeemMintPass
since attributes fighterTypes
and iconTypes
are not validated
function redeemMintPass( uint256[] calldata mintpassIdsToBurn, >> uint8[] calldata fighterTypes, >> uint8[] calldata iconsTypes, string[] calldata mintPassDnas, string[] calldata modelHashes, string[] calldata modelTypes ) external { require( mintpassIdsToBurn.length == mintPassDnas.length && mintPassDnas.length == fighterTypes.length && fighterTypes.length == modelHashes.length && modelHashes.length == modelTypes.length ); for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) { require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i])); _mintpassInstance.burn(mintpassIdsToBurn[i]); _createNewFighter( msg.sender, uint256(keccak256(abi.encode(mintPassDnas[i]))), modelHashes[i], modelTypes[i], >> fighterTypes[i], >> iconsTypes[i], [uint256(100), uint256(100)] ); } }
Coded POC for FighterFarm.t.sol
function testMintDendroid() 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 _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs); // 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"; _neuralNetHashes[0] = "neuralnethash"; _modelTypes[0] = "original"; _iconsTypes[0] = 1; // Dendroid is fighterType = 1 _fighterTypes[0] = 1; // approve the fighterfarm contract to burn the mintpass _mintPassContract.approve(address(_fighterFarmContract), 1); _fighterFarmContract.redeemMintPass( _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes ); (, uint256[6] memory attr, , , , ,) = _fighterFarmContract.getAllFighterInfo(0); // Dendroids have all their attributes set to 99 assertEq(attr[0], 99); }
Foundry
If we look in the AAMintPass.sol
we can see that users can mint passes for dendroids
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AAMintPass.sol#L109
/// @notice This allows you to claim a mintpass which you have qualified for /// @dev Users must provide the number of mintpasses they want to claim, along with the /// tokenURIs and a signature from our delegated server address. We then verify that the /// server did indeed sign a message approving them to claim the amount of mint passes. /// We use passesClaimed as a part of the message and increment it to ensure they cannot use /// the same signature multiple times. /// @param numToMint The number of mintpasses to claim. The first element in the array is the >> /// number of AI Champion mintpasses and the second element is the number of Dendroid /// mintpasses. /// @param signature The signature from the delegated server address /// @param _tokenURIs Token URIs for each of the mintpasses a user claims function claimMintPass( uint8[2] calldata numToMint, bytes calldata signature, string[] calldata _tokenURIs ) external {
But there is no way to know which mint pass is the dendroid one, because of that we should consider removing ability to mint dendroids for now.
function redeemMintPass( uint256[] calldata mintpassIdsToBurn, - uint8[] calldata fighterTypes, - uint8[] calldata iconsTypes, string[] calldata mintPassDnas, string[] calldata modelHashes, string[] calldata modelTypes ) external { require( mintpassIdsToBurn.length == mintPassDnas.length && - mintPassDnas.length == fighterTypes.length && fighterTypes.length == modelHashes.length && modelHashes.length == modelTypes.length ); for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) { require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i])); _mintpassInstance.burn(mintpassIdsToBurn[i]); _createNewFighter( msg.sender, uint256(keccak256(abi.encode(mintPassDnas[i]))), modelHashes[i], modelTypes[i], + 0, + 0, [uint256(100), uint256(100)] ); } }
Invalid Validation
#0 - c4-pre-sort
2024-02-22T07:13:35Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-22T07:13:45Z
raymondfam marked the issue as duplicate of #33
#2 - c4-pre-sort
2024-02-26T00:52:53Z
raymondfam marked the issue as duplicate of #1626
#3 - c4-judge
2024-03-05T10:53:04Z
HickupHH3 changed the severity to 3 (High Risk)
#4 - c4-judge
2024-03-05T10:53:08Z
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
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L370
Lack of validation leads to multiple consequences
Every time function incrementGeneration
is called maxRerollAllowed
for a specified fighter type is increased by 1
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L129
function incrementGeneration(uint8 fighterType) external returns (uint8) { require(msg.sender == _ownerAddress); generation[fighterType] += 1; >> maxRerollsAllowed[fighterType] += 1; return generation[fighterType]; }
Because fighter type is not validated, user can re-roll more times that he is allowed, for example type 0 fighters current gen is 3 and type 1 gen is 0. Type 1 owner can cal reRoll
with fighterType = 0
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L370
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] = ""; } }
And re-roll two more times, the trade-off is that fighter will lose it's default dendroid appearance, but since it's just a cosmetic compared to the element and weight parameters, it's a pretty good deal.
Some tweaks are needed to run the test below
silence assertion in FighterFarm.t.sol
https://github.com/code-423n4/2024-02-ai-arena/blob/main/test/FighterFarm.t.sol#L398
/// @notice Helper function to fund an account with 4k $NRN tokens. function _fundUserWith4kNeuronByTreasury(address user) internal { vm.prank(_treasuryAddress); _neuronContract.transfer(user, 4_000 * 10 ** 18); + //assertEq(4_000 * 10 ** 18 == _neuronContract.balanceOf(user), true); }
silence the elements bug https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L462
function _createFighterBase( uint256 dna, uint8 fighterType ) private view returns (uint256, uint256, uint256) { + uint256 element = 1;//dna % numElements[generation[fighterType]]; uint256 weight = dna % 31 + 65; uint256 newDna = fighterType == 0 ? dna : uint256(fighterType); return (element, weight, newDna); }
Coded POC for FighterFarm.t.sol
function testRerollExploit() public { // get 4k neuron from treasury _fundUserWith4kNeuronByTreasury(_ownerAddress); _fundUserWith4kNeuronByTreasury(_ownerAddress); _neuronContract.addSpender(address(_fighterFarmContract)); 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 _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs); // 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"; _neuralNetHashes[0] = "neuralnethash"; _modelTypes[0] = "original"; _iconsTypes[0] = 1; // Dendroid is fighterType = 1 _fighterTypes[0] = 1; // approve the fighterfarm contract to burn the mintpass _mintPassContract.approve(address(_fighterFarmContract), 1); _fighterFarmContract.redeemMintPass( _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes ); // Increase generation for type 0 _fighterFarmContract.incrementGeneration(0); _fighterFarmContract.incrementGeneration(0); // check the roll count assertEq(_fighterFarmContract.maxRerollsAllowed(0), 5); assertEq(_fighterFarmContract.maxRerollsAllowed(1), 3); // normal re-roll _fighterFarmContract.reRoll(0, 1); _fighterFarmContract.reRoll(0, 1); _fighterFarmContract.reRoll(0, 1); // revert max reroll allowance exceeded vm.expectRevert(); _fighterFarmContract.reRoll(0, 1); // re-roll with other type as an argument _fighterFarmContract.reRoll(0, 0); _fighterFarmContract.reRoll(0, 0); }
Foundry
Assert that tokenId fighter type is the same as fighterType
function reRoll(uint8 tokenId, uint8 fighterType) public { + require((fighterType == 1) == fighters[tokenId].dendroidBool, "Type mismatch"); 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] = ""; } }
Invalid Validation
#0 - c4-pre-sort
2024-02-21T23:55:52Z
raymondfam marked the issue as insufficient quality report
#1 - c4-pre-sort
2024-02-21T23:56:15Z
raymondfam marked the issue as duplicate of #17
#2 - c4-pre-sort
2024-02-22T00:36:34Z
raymondfam marked the issue as not a duplicate
#3 - c4-pre-sort
2024-02-22T00:36:39Z
raymondfam marked the issue as sufficient quality report
#4 - c4-pre-sort
2024-02-22T00:36:44Z
raymondfam marked the issue as primary issue
#5 - raymondfam
2024-02-22T00:40:33Z
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.
#6 - c4-pre-sort
2024-02-22T00:58:13Z
raymondfam marked the issue as duplicate of #306
#7 - c4-judge
2024-03-05T04:26:44Z
HickupHH3 marked the issue as satisfactory
#8 - c4-judge
2024-03-19T09:05:08Z
HickupHH3 changed the severity to 3 (High Risk)
๐ 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/main/src/FighterFarm.sol#L129-L134 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L470
It will be impossible to create fighters if generation is incremented
When a user mints or re-rolls a fighter token _createFighterBase
function is invoked
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L380
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L500
function _createFighterBase( uint256 dna, uint8 fighterType ) private view returns (uint256, uint256, uint256) { >> uint256 element = dna % numElements[generation[fighterType]]; uint256 weight = dna % 31 + 65; uint256 newDna = fighterType == 0 ? dna : uint256(fighterType); return (element, weight, newDna); }
We use the number of elements for a given generation ,that is stored in the elements
mapping, to decide the fighter element. elements[0]
is initialized in the constructor.
constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_) ERC721("AI Arena Fighter", "FTR") { _ownerAddress = ownerAddress; _delegatedAddress = delegatedAddress; treasuryAddress = treasuryAddress_; >> numElements[0] = 3; }
Unfortunately later, when the owner calls incrementGeneration
function to change generation of the specified fighter type
function incrementGeneration(uint8 fighterType) external returns (uint8) { require(msg.sender == _ownerAddress); >> generation[fighterType] += 1; maxRerollsAllowed[fighterType] += 1; return generation[fighterType]; }
elements
value is not updated for the next generation and will hold a zero, as a result all attempts to create a fighter of the new generation will revert.
Here is the coded POC for FighterFarm.t.sol
function testElementsNotUpdated() public { assertEq(_fighterFarmContract.numElements(0), 3); // increment generation for fighter type 0 _fighterFarmContract.incrementGeneration(0); // generation is set to 1 assertEq(_fighterFarmContract.generation(0), 1); // 0 elements assertEq(_fighterFarmContract.numElements(1), 0); 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"; // will revert with "Division or modulo by 0" _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes); }
Foundry
Update elements
mapping in incrementGeneration
function incrementGeneration(uint8 fighterType) external returns (uint8) { require(msg.sender == _ownerAddress); + uint8 currGeneration = generation[fighterType]; generation[fighterType] += 1; maxRerollsAllowed[fighterType] += 1; + if(elements[currGeneration + 1] == 0) + elements[currGeneration] = 3; return generation[fighterType]; }
DoS
#0 - c4-pre-sort
2024-02-22T18:19:08Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-22T18:19:23Z
raymondfam marked the issue as duplicate of #45
#2 - c4-judge
2024-03-07T06:53:22Z
HickupHH3 marked the issue as satisfactory
๐ Selected for report: nuthan2x
Also found by: 0xE1, 0xblackskull, 0xgrbr, 0xvj, Greed, McToady, MidgarAudits, PetarTolev, Sabit, SovaSlava, SpicyMeatball, Timenov, Tychai0s, _eperezok, alexxander, btk, c0pp3rscr3w3r, favelanky, jesjupyter, josephdara, juancito, klau5, kutugu, lil_eth, merlinboii, pynschon, sandy, shaflow2, zaevlad
14.5737 USDC - $14.57
Judge has assessed an item in Issue #99 as 2 risk. The relevant finding follows:
Default admin is not initialized
#0 - c4-judge
2024-03-20T02:41:12Z
HickupHH3 marked the issue as duplicate of #1507
#1 - c4-judge
2024-03-20T02:41:17Z
HickupHH3 marked the issue as partial-50
๐ Selected for report: ktg
Also found by: 0xCiphky, 0xDetermination, 0xRiO, 0xWallSecurity, 0xlemon, 0xvj, AlexCzm, BARW, Blank_Space, Draiakoo, FloatingPragma, Giorgio, Matue, McToady, MrPotatoMagic, Silvermist, SpicyMeatball, Tendency, Topmark, Tumelo_Crypto, _eperezok, agadzhalov, ahmedaghadi, alexxander, aslanbek, cats, d3e4, denzi_, dutra, evmboi32, fnanni, givn, handsomegiraffe, haxatron, immeas, juancito, ke1caM, kiqo, klau5, krikolkk, niser93, peter, petro_1912, pkqs90, rspadi, sl1, stakog, visualbits, vnavascues, yotov721
1.876 USDC - $1.88
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L503-L504 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L142
The user can specify an element and weight that is far beyond the allowed boundaries.
The round winner can mint a fighter token as a reward from the MergingPool.sol
function claimRewards( string[] calldata modelURIs, string[] calldata modelTypes, >> uint256[2][] calldata customAttributes ) external { uint256 winnersLength; uint32 claimIndex = 0; uint32 lowerBound = numRoundsClaimed[msg.sender]; for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) { numRoundsClaimed[msg.sender] += 1; winnersLength = winnerAddresses[currentRound].length; for (uint32 j = 0; j < winnersLength; j++) { if (msg.sender == winnerAddresses[currentRound][j]) { _fighterFarmInstance.mintFromMergingPool( msg.sender, modelURIs[claimIndex], modelTypes[claimIndex], >> customAttributes[claimIndex] ); claimIndex += 1; } } } if (claimIndex > 0) { emit Claimed(msg.sender, claimIndex); } }
Function claimRewards
accept any values for customAttributes
, later these attributes are used to initialize the element and weight of the fighter
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L329
function _createNewFighter( address to, uint256 dna, string memory modelHash, string memory modelType, uint8 fighterType, uint8 iconsType, uint256[2] memory customAttributes ) private { require(balanceOf(to) < MAX_FIGHTERS_ALLOWED); uint256 element; uint256 weight; uint256 newDna; if (customAttributes[0] == 100) { (element, weight, newDna) = _createFighterBase(dna, fighterType); } else { >> element = customAttributes[0]; >> weight = customAttributes[1]; newDna = dna; }
Although there are certain boundaries that are set for the element and weight values, https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L470-L471
uint256 element = dna % numElements[generation[fighterType]]; uint256 weight = dna % 31 + 65;
they are bypassed if we use merging pool to mint a fighter. Since it is unknown how backend will process values that are outside the limits, I rate the severity of this bug as medium.
Coded POC for FighterFarm.t.sol
function testCustomAttributes() public { vm.startPrank(address(_mergingPoolContract)); uint256 MAX = type(uint256).max; _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [MAX, MAX]); // let's see if we got the hat (, , uint256 weight, uint256 element, , ,) = _fighterFarmContract.getAllFighterInfo(0); assertEq(weight, MAX); assertEq(element, MAX); }
Foundry
Validate customAttributes
Invalid Validation
#0 - c4-pre-sort
2024-02-24T08:51:36Z
raymondfam marked the issue as insufficient quality report
#1 - c4-pre-sort
2024-02-24T08:51:48Z
raymondfam marked the issue as duplicate of #226
#2 - c4-judge
2024-03-11T10:19:04Z
HickupHH3 marked the issue as satisfactory
๐ Selected for report: ahmedaghadi
Also found by: 0x13, 0xAleko, 0xDetermination, 0xKowalski, 0xPluto, 0xRiO, 0xvj, AlexCzm, Avci, BARW, BigVeezus, Cryptor, DeFiHackLabs, Draiakoo, Fitro, Giorgio, GoSlang, Greed, Honour, Kalogerone, KmanOfficial, Krace, McToady, MidgarAudits, MrPotatoMagic, Nyxaris, ReadyPlayer2, Ryonen, SovaSlava, SpicyMeatball, VAD37, _eperezok, alexzoid, almurhasan, btk, cu5t0mpeo, deadrxsezzz, djxploit, dvrkzy, emrekocak, erosjohn, evmboi32, fnanni, grearlake, inzinko, jesjupyter, jesusrod15, josephdara, ke1caM, klau5, ktg, ladboy233, merlinboii, nuthan2x, peanuts, pipidu83, pontifex, radev_sw, sl1, sobieski, soliditywala, t0x1c, taner2344, vnavascues, y4y, yovchev_yoan, zaevlad
0.2347 USDC - $0.23
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L148-L163 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L495
Winner can't claim his reward from the merging pool if the reward exceeds MAX_FIGHTERS_ALLOWED = 10.
Every round the merging pool admin decides a certain amount of winners, these addresses can mint a fighter token as a reward
function claimRewards( string[] calldata modelURIs, string[] calldata modelTypes, uint256[2][] calldata customAttributes ) external { uint256 winnersLength; uint32 claimIndex = 0; uint32 lowerBound = numRoundsClaimed[msg.sender]; for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) { numRoundsClaimed[msg.sender] += 1; winnersLength = winnerAddresses[currentRound].length; for (uint32 j = 0; j < winnersLength; j++) { if (msg.sender == winnerAddresses[currentRound][j]) { >> _fighterFarmInstance.mintFromMergingPool( msg.sender, modelURIs[claimIndex], modelTypes[claimIndex], customAttributes[claimIndex] ); claimIndex += 1; } } }
A situation may arise when a user hasn't claimed his rewards for a long time, and as a result, the amount of claimable tokens has become greater than the maximum allowed fighters amount for one address.
Coded POC for MerginPool.t.sol
function testClaimRewardsFail() public { _mintFromMergingPool(_ownerAddress); _mintFromMergingPool(_DELEGATED_ADDRESS); // 10 round winning streak just to prove the bug, in reality there will be other winners uint256[] memory _winners = new uint256[](2); _winners[0] = 0; _winners[1] = 1; string[] memory _modelURIs = new string[](10); string[] memory _modelTypes = new string[](10); uint256[2][] memory _customAttributes = new uint256[2][](10); for(uint256 i=0; i<10; i++) { _modelURIs[i] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m"; _modelTypes[i] = "original"; _customAttributes[i][0] = uint256(1); _customAttributes[i][1] = uint256(80); _mergingPoolContract.pickWinner(_winners); } // winner claims rewards for previous roundIds vm.expectRevert(); _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes); }
Foundry
Allow user to choose how many rewards he wants to claim in one transaction.
Token-Transfer
#0 - c4-pre-sort
2024-02-22T08:38:51Z
raymondfam marked the issue as insufficient quality report
#1 - c4-pre-sort
2024-02-22T08:41:18Z
raymondfam marked the issue as sufficient quality report
#2 - c4-pre-sort
2024-02-22T08:41:31Z
raymondfam marked the issue as duplicate of #216
#3 - c4-judge
2024-03-11T12:48:32Z
HickupHH3 marked the issue as satisfactory
๐ Selected for report: ahmedaghadi
Also found by: 0x13, 0xAleko, 0xDetermination, 0xKowalski, 0xPluto, 0xRiO, 0xvj, AlexCzm, Avci, BARW, BigVeezus, Cryptor, DeFiHackLabs, Draiakoo, Fitro, Giorgio, GoSlang, Greed, Honour, Kalogerone, KmanOfficial, Krace, McToady, MidgarAudits, MrPotatoMagic, Nyxaris, ReadyPlayer2, Ryonen, SovaSlava, SpicyMeatball, VAD37, _eperezok, alexzoid, almurhasan, btk, cu5t0mpeo, deadrxsezzz, djxploit, dvrkzy, emrekocak, erosjohn, evmboi32, fnanni, grearlake, inzinko, jesjupyter, jesusrod15, josephdara, ke1caM, klau5, ktg, ladboy233, merlinboii, nuthan2x, peanuts, pipidu83, pontifex, radev_sw, sl1, sobieski, soliditywala, t0x1c, taner2344, vnavascues, y4y, yovchev_yoan, zaevlad
0.2347 USDC - $0.23
Judge has assessed an item in Issue #99 as 2 risk. The relevant finding follows:
The user risks receiving an OOG error if he doesnโt claim NRN rewards for too long.
#0 - c4-judge
2024-03-12T02:58:34Z
HickupHH3 marked the issue as duplicate of #216
#1 - c4-judge
2024-03-12T02:58:38Z
HickupHH3 marked the issue as partial-25
#2 - c4-judge
2024-03-21T03:03:25Z
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/main/src/AiArenaHelper.sol#L107 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L214 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L254 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L324 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L379
The user can easily predict which dna
value will give his fighter rare attributes and adjust the input accordingly.
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L510
FighterOps.FighterPhysicalAttributes memory attrs = _aiArenaHelperInstance.createPhysicalAttributes( >> newDna, generation[fighterType], iconsType, dendroidBool );
There are three ways to create a fighter in the FighterFarm
contract:
Physical attributes of the newly created fighter depend on the dna
value. First we calculate a rarity
value from the dna
that is a number from 0 to 100.
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L107
function createPhysicalAttributes( uint256 dna, uint8 generation, uint8 iconsType, bool dendroidBool ) external view returns (FighterOps.FighterPhysicalAttributes memory) { ... } else { >> uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100; >> uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]); finalAttributeProbabilityIndexes[i] = attributeIndex; } }
Then we use rarity
and probability array to decide the attribute
function dnaToIndex(uint256 generation, uint256 rarityRank, string memory attribute) public view returns (uint256 attributeProbabilityIndex) { uint8[] memory attrProbabilities = getAttributeProbabilities(generation, attribute); uint256 cumProb = 0; uint256 attrProbabilitiesLength = attrProbabilities.length; for (uint8 i = 0; i < attrProbabilitiesLength; i++) { cumProb += attrProbabilities[i]; if (cumProb >= rarityRank) { >> attributeProbabilityIndex = i + 1; break; } } return attributeProbabilityIndex; }
For example, if we have probability array [60, 30, 10], to get third item that has probability 10%, we need to roll numbers 90-100 ( 90<= rarity <=100).
Now to our minter functions, let's start with the easiest one redeemMintPass
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L233-L262
User can directly input the dna
value
function redeemMintPass( uint256[] calldata mintpassIdsToBurn, uint8[] calldata fighterTypes, uint8[] calldata iconsTypes, string[] calldata mintPassDnas, string[] calldata modelHashes, string[] calldata modelTypes ) external { ... for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) { require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i])); _mintpassInstance.burn(mintpassIdsToBurn[i]); _createNewFighter( msg.sender, >> uint256(keccak256(abi.encode(mintPassDnas[i]))), modelHashes[i], modelTypes[i], fighterTypes[i], iconsTypes[i], [uint256(100), uint256(100)] ); } }
Let's say I want that rare hat with 5% probability for my fighter, probability array is [5, 25, 25, 50], and divisor for the head attribute is 2. All I need is to find a string that will produce a hash (hash / 2) % 100 = 0..5
.
Other methods are more tricky and use msg.sender
and fighters
array length to calculate the dna
, but they still can be manipulated to some extent, which I will demonstrate in the coded POC.
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L214
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L324
This one is a re-roll
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L379
Coded POC for FighterFarm.t.sol
redeemMintPass
function testGetTheCoolHat() 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 _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs); // 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; _fighterTypes[0] = 0; _neuralNetHashes[0] = "neuralnethash"; _modelTypes[0] = "original"; _iconsTypes[0] = 1; // head probabilities array [25, 25, 13, 13, 9, 9] // let's get that 9% item, we need to roll 85-100 uint256 coolHat = 6; // this will do _mintPassDNAs[0] = "blahblahblah"; uint256 hash = uint256(keccak256(abi.encode(_mintPassDNAs[0]))); console.log("DNA: ", hash); console.log("RAR: ", (hash / 2) % 100); // approve the fighterfarm contract to burn the mintpass _mintPassContract.approve(address(_fighterFarmContract), 1); _fighterFarmContract.redeemMintPass( _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes ); // let's see if we got the hat (, uint256[6] memory attr, , , , ,) = _fighterFarmContract.getAllFighterInfo(0); assertEq(attr[0], coolHat); }
mintFromMergingPool
function testGetTheCoolHatPartDeux() public { // head probabilities array [25, 25, 13, 13, 9, 9] // let's get that 9% item, we need to roll 85-100 uint256 coolHat = 6; // this one is harder, msg.sender is always the same = merging pool, so we need to rely on figters array length // length of 2 is good! uint256 hash = uint256(keccak256(abi.encode(address(_mergingPoolContract), 2))); console.log("DNA: ", hash); console.log("RAR: ", (hash / 2) % 100); vm.startPrank(address(_mergingPoolContract)); // wait for other people to mint 2 tokens _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(1), uint256(80)]); _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(1), uint256(80)]); // now it's our turn _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(1), uint256(80)]); // let's see if we got the hat (, uint256[6] memory attr, , , , ,) = _fighterFarmContract.getAllFighterInfo(2); assertEq(attr[0], coolHat); }
Foundry
Make rarity
value harder to predict, ideally chainlink VRF should be used, but if it's not possible we need to throw more parameters into the keccak256 algorithm.
For example
function mintFromMergingPool( address to, string calldata modelHash, string calldata modelType, uint256[2] calldata customAttributes ) public { require(msg.sender == _mergingPoolAddress); _createNewFighter( to, + uint256(keccak256(abi.encode(msg.sender, fighters.length, to, modelHash, modelType, customAttributes))), modelHash, modelType, 0, 0, customAttributes ); }
Other
#0 - c4-pre-sort
2024-02-24T01:28:29Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-24T01:28:54Z
raymondfam marked the issue as duplicate of #53
#2 - c4-judge
2024-03-06T03:47:46Z
HickupHH3 marked the issue as satisfactory
#3 - c4-judge
2024-03-06T03:49:26Z
HickupHH3 changed the severity to 3 (High Risk)
#4 - c4-judge
2024-03-20T01:04:09Z
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
1.0089 USDC - $1.01
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/StakeAtRisk.sol#L102-L104 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L461-L462
If amountLost[user]
is smaller than stakeAtRisk[roundId][fighterId]
, NRN reclaim is not possible, and as a result record update for the win condition will always revert.
Imagine a situation: Alice staked 1000 NRN to the fighter #1, after a sequence of defeats she ended up with 500 NRN at risk. Later, she decided to unstake what was left and sell her fighter. Bob buys it from her in hopes of receiving NRN that is currently at risk for himself.
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/StakeAtRisk.sol#L45-L49
The current state in round 0 is: amountLost[bob] = 0
, stakeAtRisk[0][1] = 500
.
Bob wins his first match with the fighter #1 and server calls the updateBattleRecord
function
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L322
function updateBattleRecord( uint256 tokenId, uint256 mergingPortion, uint8 battleResult, uint256 eloFactor, bool initiatorBool ) external { require(msg.sender == _gameServerAddress); require(mergingPortion <= 100); address fighterOwner = _fighterFarmInstance.ownerOf(tokenId); require( !initiatorBool || _voltageManagerInstance.ownerVoltageReplenishTime(fighterOwner) <= block.timestamp || _voltageManagerInstance.ownerVoltage(fighterOwner) >= VOLTAGE_COST ); _updateRecord(tokenId, battleResult); uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId); >> if (amountStaked[tokenId] + stakeAtRisk > 0) { >> _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner); } if (initiatorBool) { _voltageManagerInstance.spendVoltage(fighterOwner, VOLTAGE_COST); } totalBattles += 1; }
Stake at risk is > 0, contract will try to call _addResultPoints
. Unfortunately this internal call will revert with underflow because risk values for token and owner are not the same
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L460-L461
function reclaimNRN(uint256 nrnToReclaim, uint256 fighterId, address fighterOwner) external { ... >> stakeAtRisk[roundId][fighterId] -= nrnToReclaim; totalStakeAtRisk[roundId] -= nrnToReclaim; >> amountLost[fighterOwner] -= nrnToReclaim; emit ReclaimedStake(fighterId, nrnToReclaim); } }
Coded POC for RankedBattle.t.sol
function testWinFail() public { address alice = vm.addr(3); address bob = vm.addr(4); _mintFromMergingPool(alice); uint8 tokenId = 0; _fundUserWith4kNeuronByTreasury(alice); vm.prank(alice); _rankedBattleContract.stakeNRN(1_000 * 10 ** 18, tokenId); vm.prank(address(_GAME_SERVER_ADDRESS)); _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true); uint256 atRisk = _stakeAtRiskContract.getStakeAtRisk(tokenId); assertEq(atRisk, 1e18); // alice unstakes and sells the token vm.startPrank(alice); _rankedBattleContract.unstakeNRN(999 * 10 ** 18, tokenId); _fighterFarmContract.transferFrom(alice, bob, tokenId); vm.stopPrank(); // bob's win reverts with underflow vm.prank(address(_GAME_SERVER_ADDRESS)); vm.expectRevert(); _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, true); }
Foundry
Perhaps we can modify reclaimNRN
function
function reclaimNRN(uint256 nrnToReclaim, uint256 fighterId, address fighterOwner) external { require(msg.sender == _rankedBattleAddress, "Call must be from RankedBattle contract"); require( stakeAtRisk[roundId][fighterId] >= nrnToReclaim, "Fighter does not have enough stake at risk" ); bool success = _neuronInstance.transfer(_rankedBattleAddress, nrnToReclaim); if (success) { stakeAtRisk[roundId][fighterId] -= nrnToReclaim; totalStakeAtRisk[roundId] -= nrnToReclaim; + if(amountLost[fighterOwner] < nrnToReclaim) + amountLost[fighterOwner] = 0; emit ReclaimedStake(fighterId, nrnToReclaim); } }
DoS
#0 - c4-pre-sort
2024-02-24T04:15:36Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-24T04:16:00Z
raymondfam marked the issue as duplicate of #1641
#2 - c4-judge
2024-03-12T04:01:25Z
HickupHH3 changed the severity to 3 (High Risk)
#3 - c4-judge
2024-03-12T04:03:31Z
HickupHH3 changed the severity to 2 (Med Risk)
#4 - c4-judge
2024-03-12T06:45:01Z
HickupHH3 marked the issue as satisfactory
๐ Selected for report: t0x1c
Also found by: 0xCiphky, 0xDetermination, Draiakoo, Greed, Kalogerone, MatricksDeCoder, MidgarAudits, MrPotatoMagic, PedroZurdo, Shubham, SpicyMeatball, VAD37, Velislav4o, ZanyBonzy, btk, cats, djxploit, forkforkdog, givn, ladboy233, lanrebayode77, lil_eth, visualbits, zaevlad
29.6169 USDC - $29.62
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol#L312-L314
The user can change address and mint tokens beyond daily allowance that is set for one account.
Users are allowed to mint only certain amount of tokens each day, this allowance is replenished after 24 hours.
function mint(uint256 tokenId, uint256 quantity) external { require(tokenId < _itemCount); uint256 price = allGameItemAttributes[tokenId].itemPrice * quantity; require(_neuronInstance.balanceOf(msg.sender) >= price, "Not enough NRN for purchase"); require( allGameItemAttributes[tokenId].finiteSupply == false || ( allGameItemAttributes[tokenId].finiteSupply == true && quantity <= allGameItemAttributes[tokenId].itemsRemaining ) ); require( >> dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp || >> quantity <= allowanceRemaining[msg.sender][tokenId] );
Unfortunately this restriction is pretty easy to bypass by calling the mint
function from another address.
Coded POC for GameItems.t.sol
function testBypassAllowance() public { address anotherAddress = address(0x2); assertEq(_gameItemsContract.getAllowanceRemaining(_ownerAddress, 0), 10); _fundUserWith4kNeuronByTreasury(_ownerAddress); _fundUserWith4kNeuronByTreasury(anotherAddress); // mint from account 1 _gameItemsContract.mint(0, 10); assertEq(_gameItemsContract.getAllowanceRemaining(_ownerAddress, 0), 0); // and again from another account, 20 tokens instead of 10 vm.prank(anotherAddress); _gameItemsContract.mint(0, 10); }
Foundry
Use a global allowance counter instead of a personal one.
Token-Transfer
#0 - c4-pre-sort
2024-02-22T17:55:55Z
raymondfam marked the issue as insufficient quality report
#1 - c4-pre-sort
2024-02-22T17:56:02Z
raymondfam marked the issue as duplicate of #43
#2 - c4-judge
2024-03-07T04:15:24Z
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
transferOwnership
doesn't revoke admin rights from the old ownerhttps://github.com/code-423n4/2024-02-ai-arena/blob/main/src/VoltageManager.sol#L64 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L85 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol#L108 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L89 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L167
The owner is granted admin rights when the contract is created
constructor(address ownerAddress, address gameItemsContractAddress) { _ownerAddress = ownerAddress; _gameItemsContractInstance = GameItems(gameItemsContractAddress); >> isAdmin[_ownerAddress] = true; }
When transferring ownership to another address, the old owner still retains that role. Consider setting isAdmin
for the old owner to false
.
Initiator of the ranked battle needs to spend 10 voltage units to participate https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L345-L347
function spendVoltage(address spender, uint8 voltageSpent) public { require(spender == msg.sender || allowedVoltageSpenders[msg.sender]); if (ownerVoltageReplenishTime[spender] <= block.timestamp) { _replenishVoltage(spender); } ownerVoltage[spender] -= voltageSpent; emit VoltageRemaining(spender, ownerVoltage[spender]); }
The voltage is replenished after 24 hours or by sacrificing a special NFT https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/VoltageManager.sol#L117
function _replenishVoltage(address owner) private { >> ownerVoltage[owner] = 100; ownerVoltageReplenishTime[owner] = uint32(block.timestamp + 1 days); }
As we can see, the voltage is bound to the fighter owner's address, thus if owner has multiple fighters, he can split them between his wallets, increasing the number of matches he can participate in during the day. This includes staked and unstaked matches.
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L191-L206 Users are allowed to mint a fighter token by providing a signature from an address owned by the protocol. If Ai-Arena will deploy in other networks in the future, this signature can be replayed.
FighterFarm.sol
lacks setter for the _delegatedAddress
_delegatedAddress
is used to sign fighter claim messages and setting token URIs, but unlike other instances such as NeuronToken
or MergingPool
, it cannot be changed.
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L45-L51
constructor(uint8[][] memory probabilities) { _ownerAddress = msg.sender; // Initialize the probabilities for each attribute >> addAttributeProbabilities(0, probabilities); uint256 attributesLength = attributes.length; >> for (uint8 i = 0; i < attributesLength; i++) { attributeProbabilities[0][attributes[i]] = probabilities[i]; attributeToDnaDivisor[attributes[i]] = defaultAttributeDivisor[i]; } }
addAttributeProbabilities(0, probabilities)
performs the same function as the for loop below.
addAttributeDivisor
functionhttps://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L74
Since we are using elements attributeToDnaDivisor
elements as divisors, additional check is needed to validate that these elements !=0
uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
addAttributeProbabilities
can use more validationhttps://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L131
attributeProbabilities
elements correspond to the likelihood of acquiring a particular physical attribute. If the sum of the elements exceeds 100, some attributes will not be available.
for (uint8 i = 0; i < attrProbabilitiesLength; i++) { cumProb += attrProbabilities[i]; if (cumProb >= rarityRank) { // rarityRank is a number 0-100 attributeProbabilityIndex = i + 1; break; } }
viewFighterInfo
can be more verbosehttps://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterOps.sol#L79
viewFighterInfo
function doesn't return the fighter type or icon type.
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L68-L76 Neuron token inherits from the OZ AccessControl contract, instead of using DEFAULT_ADMIN_ROLE it relies on custom functions like the one below
function addMinter(address newMinterAddress) external { require(msg.sender == _ownerAddress); >> _setupRole(MINTER_ROLE, newMinterAddress); }
This makes role management less flexible because there is no way to revoke roles if needed. Consider granting DEFAULT_ADMIN_ROLE to the owner in the constructor.
setupAirdrop
can overwrite unclaimed approvalshttps://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L127-L134
function setupAirdrop(address[] calldata recipients, uint256[] calldata amounts) external { require(isAdmin[msg.sender]); require(recipients.length == amounts.length); uint256 recipientsLength = recipients.length; for (uint32 i = 0; i < recipientsLength; i++) { >> _approve(treasuryAddress, recipients[i], amounts[i]); } }
If the airdrop consists of multiple stages, users who do not claim their tokens using claim
function risk having their approval overwritten in the next stage.
// OZ ERC20 contract function _approve( address owner, address spender, uint256 amount ) internal virtual { require(owner != address(0), "ERC20: approve from the zero address"); require(spender != address(0), "ERC20: approve to the zero address"); >> _allowances[owner][spender] = amount; emit Approval(owner, spender, amount); }
Consider using increaseAllowance
instead.
burnFrom
shouldn't decrease allowance in case it's type(uint256).max
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L201
If spender has an infinite allowance we can burn tokens without changing _allowances
value.
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L220
setRankedNrnDistribution
takes the NRN amount without the 10 ** 18 multiplier, most Dapps use X * 10 ** (decimal) amounts as arguments and it's pretty easy to make a mistake and enter such a value into setRankedNrnDistribution
too.
globalStake
value is not updated if staker loses his stake in battleglobalStake
variable is used to track total amount of NRN tokens that was staked in the RankedBattle.sol
contract.
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L257
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L276
Unfortunately it's is not updated when part of the stake that was at risk during a round is transferred to the treasury at the end of the round.
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L491-L498
} else { /// If the fighter does not have any points for this round, NRNs become at risk of being lost bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk); if (success) { _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner); amountStaked[tokenId] -= curStakeAtRisk; } } }
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/StakeAtRisk.sol#L142
/// @notice Sweeps the lost stake to the treasury contract. /// @dev This function is called internally to transfer the lost stake to the treasury contract. function _sweepLostStake() private returns(bool) { return _neuronInstance.transfer(treasuryAddress, totalStakeAtRisk[roundId]); }
NOT MENTIONED IN https://github.com/code-423n4/2024-02-ai-arena/blob/main/bot-report.md#l-05-external-calls-in-an-un-bounded-for-loop-may-result-in-a-dos
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L299-L305
claimNRN
function iterates through all rounds in which user didn't claim to calculate the final reward, if a substantial number of rounds has passed since the last claim, the function can revert with an out of gas error.
uint32 lowerBound = numRoundsClaimed[msg.sender]; for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) { nrnDistribution = getNrnDistribution(currentRound); claimableNRN += ( accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution ) / totalAccumulatedPoints[currentRound]; numRoundsClaimed[msg.sender] += 1; }
Consider allowing the user to specify the number of rounds for which he would like to be rewarded.
#0 - raymondfam
2024-02-26T07:29:35Z
The user risks receiving an OOG error if he doesnโt claim NRN rewards for too long.: to #1541
#1 - c4-pre-sort
2024-02-26T07:29:40Z
raymondfam marked the issue as sufficient quality report
#2 - HickupHH3
2024-03-08T03:54:47Z
#267: L
#3 - c4-judge
2024-03-14T07:24:34Z
HickupHH3 marked the issue as grade-b