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: 86/283
Findings: 8
Award: $65.83
π Selected for report: 0
π Solo Findings: 0
π 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
There is only exist 2 fighterType. 0 for premium fighter with custom DNA, 1 for default fighter.
FighterFarm.redeemMintPass()
allow user input anything as long as they have enough mint pass to burn.
This include FighterType
id which can be any number.
FighterFarm._createNewFighter()
does not fail when fighterType > 1
.
Mint will success, there will exist an NFT with invalid fighterType
.
The only one might have problem with this is game client or gameserver handling battle simulation might not have any logic to handle non exist fighterType. Causing unknown problem.
Redeem have no input verification https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L233-L261
FighterType can only be 1 or 0. https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L127
Fighter DNA will be fighterType
id
https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L462-L474
generation
will return 0 and numsElement[]
will always be 3.
There is nothing to prevent minting fighterType > 1
from failing.
Add require check for Fighter Type
function _createNewFighter( address to, uint256 dna, //@hash from msg.sender . can be manipulated string memory modelHash,//@user ipfs url string memory modelType, //@user "original" uint8 fighterType,//0 or 1 //@audit M redeem user can use non exist fighter type to bypass DNA _createFighterBase() uint8 iconsType,//0 uint256[2] memory customAttributes//@[uint256(100), uint256(100)] default value or from merging pools ) private { require(fighterType < 2);//@ require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);
Context
#0 - c4-pre-sort
2024-02-22T08:07:48Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-22T08:07:56Z
raymondfam marked the issue as duplicate of #33
#2 - c4-pre-sort
2024-02-26T00:53:58Z
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:35:57Z
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
User of wrong fighterType
can reroll beyond limit of maxRerollsAllowed[fighterType]
.
There is only 2 fighters type ID.
/// @param fighterType Type of fighter either 0 or 1 (champion or dendroid).
Each type can reroll max 3 times.
/// @notice The maximum amount of rerolls for each fighter. uint8[2] public maxRerollsAllowed = [3, 3];
After admin increase generation, reroll limit increase by 1. https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L125-L134
So if admin increase generation of champion to 10, while dendroid is 5. Champion can reroll 10 times. While dendroid can reroll 5 times.
Because reRoll()
function does not check if token have correct fighterType as user input.
Dendroid User can reroll() 10 times with this input fighterType = Champion
despite being wrong type.
https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L370-L391
Change fighterType to enum.
/// @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));//@audit can reroll despite wrong fighter type require( (fighters[tokenId].dendroidBool?1:0) == (fighterType), "Fighter type does not match"); }
Other
#0 - c4-pre-sort
2024-02-22T02:32:11Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-22T02:32:18Z
raymondfam marked the issue as duplicate of #306
#2 - c4-judge
2024-03-05T04:36:41Z
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
https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L530-L532 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L519-L534 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L438-L439
10 user stake 1e1 (1wei) token earn the same amount total points as single user stake 100e18 $NRN. 100 user stake 1e1 (1wei) token earn the same amount total points as single user stake 10_000e18 $NRN.
Rewards distribution per round spread evenly for all player based on points. This leads to unfair rewards distribution.
Also because of rounding issue, user who deposit less than 1_000 token never get their stake removed when lost battle. Basically encourage user to stake 1 wei token for entire round.
They will get same amount of points on ranking same as user who put actual money on staking.
Rewards distribute evenly per round for all player based on points. Points can be earned by staking 1 wei token have same weight as staking 2e18 token.
Basically free points and rewards for doing absolutely nothing and not putting any serious money at risk.
Rewards distribution per round is based on points ranking. The more point you have, the more rewards you get.
Only user who have token staked allowed to earn points after battle
//https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L341-L344 uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);//@stake always 0 after 1st stake if (amountStaked[tokenId] + stakeAtRisk > 0) { _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner); }//@audit user with no stake cannot add resultPoints. There is never points increase.
Look at stake function. There is no minimum stake required. https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L241-L265
And how points earned is calculated based on StakingFactor
and elo.
//https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L416-L500 /// Check how many NRNs the fighter has at risk stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId); /// Calculate the staking factor if it has not already been calculated for this round if (_calculatedStakingFactor[tokenId][roundId] == false) { stakingFactor[tokenId] = _getStakingFactor(tokenId, stakeAtRisk);//@minimum 1 as staking factor _calculatedStakingFactor[tokenId][roundId] = true;//@ staking factor decimal is 0. if deposit 100e18. staking factor is 100^0.5=10 } // ....... /// If the user has no NRNs at risk, then they can earn points if (stakeAtRisk == 0) { points = stakingFactor[tokenId] * eloFactor; }
There is problem with StakingFactor
calculation.
Deposit 100e18 token give you square root of 100=10
staking factor.
Deposit 2e18 token give you square root of 2=1
staking factor.
Deposit 1e1 token give you 0 staking factor and rounded up to 1.
It always rounding up to 1 when deposit staking token is less than 2e18.
function _getStakingFactor( uint256 tokenId, uint256 stakeAtRisk ) private view returns (uint256) { uint256 stakingFactor_ = FixedPointMathLib.sqrt( (amountStaked[tokenId] + stakeAtRisk) / 10**18//@audit R getstaking factor seem manipulated by staking <1e18 ); if (stakingFactor_ == 0) { stakingFactor_ = 1;//@ staking always round up to 1 } return stakingFactor_; }
This mean user deposit 1e1 token have same staking factor as user deposit 1e18 token. They allowed to earn points and rewards based on points earned.
Also due to another rounding issue.
/// Potential amount of NRNs to put at risk or retrieve from the stake-at-risk contract curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;//@div 10000 //@audit H no minimum staking amount result in never get stake at risk
User who deposit less than 1_000 token never get their stake at risk due to curStakeAtRisk == 0
.
This in turn prevent contract from remove their stake when they lose battle.
https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L491-L498
This incentived cheating user to deposit 1 wei every round to earn points and some token rewards. It cost nothing and still earn rewards. As automatic battle and game server will keep sending battle report to update for all users.
Add minimum stake required to earn points and rewards.
uint256 public minimumDeposit = 10e18; function setMinimumDeposit(uint256 _minimumDeposit) external onlyOwner { minimumDeposit = _minimumDeposit; } function stakeNRN(uint256 amount, uint256 tokenId) external {//@audit can stake for ongoing round. This mean when battle going for halfway. User can still stake for winner? require(amount > minimumDeposit, "Amount cannot less than minimumDeposit");
Math
#0 - c4-pre-sort
2024-02-22T17:12:26Z
raymondfam marked the issue as insufficient quality report
#1 - c4-pre-sort
2024-02-22T17:13:09Z
raymondfam marked the issue as duplicate of #38
#2 - c4-judge
2024-03-07T02:49:49Z
HickupHH3 changed the severity to 2 (Med Risk)
#3 - c4-judge
2024-03-07T02:58:22Z
HickupHH3 changed the severity to 3 (High Risk)
#4 - c4-judge
2024-03-07T03:37:42Z
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/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L470 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L129-L134
Array numElements[]
is init once in FighterFarm.sol
constructor. But there is no function to update it.
Causing dna % numElements[generation[fighterType]];
to always revert later when generation is increase by 1.
numElements
will always return 0. Modular operation % 0
always revert.
Math division by 0. It will always revert when minting new NFT for non-zero generation.
Inside FighterFarm.sol
constructor, numElements
is init once only for generation 0.
https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L104-L111
Admin can increase generation later. https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L129-L134
When generation[fighterType] > 0
. numElements[1]
will return 0.
and this will revert.
uint256 element = dna % numElements[generation[fighterType]];
NFT minting calling _createFighterBase()
will just fail.
https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L499-L501
Prevent further NFT minting
https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L470 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L129-L134
/// @notice Increase the generation of the specified fighter type. /// @dev Only the owner address is authorized to call this function. /// @param fighterType Type of fighter either 0 or 1 (champion or dendroid). /// @return Generation count of the fighter type. function incrementGeneration(uint8 fighterType, uint newNumElements) external returns (uint8) { require(msg.sender == _ownerAddress); generation[fighterType] += 1;//@audit H admin increase generation of fighter type will destroy minting new fighter of that type due to element not exist for new generation maxRerollsAllowed[fighterType] += 1; + numElements[generation[fighterType]] = newNumElements; return generation[fighterType]; }
Math
#0 - c4-pre-sort
2024-02-22T19:06:40Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-22T19:06:48Z
raymondfam marked the issue as duplicate of #45
#2 - c4-judge
2024-03-08T03:18:49Z
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/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L292-L311 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/MergingPool.sol#L139-L167
RankedBattle.claimNRN()
and MergingPool.claimRewards()
code are gas inefficient that loop through all rounds to find rewards to claim.
For L2 game, if the game start new round every 1 hour. After 6000 rounds, or 245 days, new user claiming rewards transaction will reach 30M block gas limit.
After 6000 rounds, no new user will be able to claim rewards anymore. Required gas to succeed transaction will be higher than block gas limit.
Because after user stake token. Stake will carry on to new rounds. User can passively earn points if someone battle them. So looping through every single round to check if user have rewards that round make sense
But it extremely really inefficient logic.
This loop run every time new user claim rewards. This mean if current round is 500. New user have to loop through 500 previous rounds to check if they have any reward.
Running below foundry test to simulate this. After 6000 rounds, it cost user 30M gas to just call claimNRN()
the first time.
Same things with MergingPool.claimRewards()
. Same logic, same problem.
Modify some file to run these test.
test\RankedBattle.t.sol
/// @notice Test 2 accounts staking, winning a battle, setting a new round and claiming NRN for the previous round. function testClaimNRNGas() public { //Set round to 4000 _rankedBattleContract.debugSetNewRound(6000); address staker = vm.addr(3); //print gas report uint gasLeft = gasleft(); vm.prank(staker); _rankedBattleContract.claimNRN(); console.log("Gas used: ", gasLeft - gasleft()); }
src\RankedBattle.sol
function debugSetNewRound(uint setRound) external { require(isAdmin[msg.sender]); roundId = setRound; _stakeAtRiskInstance.setNewRound(roundId);//@audit-ok M global staked does not update when stake at risk is removed rankedNrnDistribution[roundId] = rankedNrnDistribution[roundId - 1]; } //@Edit function claimNRN() external { require(numRoundsClaimed[msg.sender] < roundId, "Already claimed NRNs for this period");//@audit M out of gas claimNRN after 4000 rounds. Tested through foundry uint256 claimableNRN = 0; uint256 nrnDistribution; uint32 lowerBound = numRoundsClaimed[msg.sender]; for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) { nrnDistribution = getNrnDistribution(currentRound); if(totalAccumulatedPoints[currentRound]>0) claimableNRN += ( accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution ) / totalAccumulatedPoints[currentRound]; else claimableNRN += 1; //@for debug gas numRoundsClaimed[msg.sender] += 1; } if (claimableNRN > 0) { amountClaimed[msg.sender] += claimableNRN; _neuronInstance.mint(msg.sender, claimableNRN); emit Claimed(msg.sender, claimableNRN); } }
Running 1 test for test/RankedBattle.t.sol:RankedBattleTest [PASS] testClaimNRNGas() (gas: 31636965) Logs: Gas used: 31550102
https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L292-L311 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/MergingPool.sol#L139-L167
Limit loop to 1000 per transaction. Or force user claimNRN every time stake or unstake.
/// @notice Claims NRN tokens for the specified rounds. /// @dev Caller can only claim once per round. function claimNRN() external { require(numRoundsClaimed[msg.sender] < roundId, "Already claimed NRNs for this period");//@audit M out of gas claimNRN after 4000 rounds. Tested through foundry uint256 claimableNRN = 0; uint256 nrnDistribution; uint32 lowerBound = numRoundsClaimed[msg.sender]; + uint roundLoop = math.Max(roundId, lowerBound + 1000); + for (uint32 currentRound = lowerBound; currentRound < roundLoop; currentRound++) { nrnDistribution = getNrnDistribution(currentRound); claimableNRN += ( accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution ) / totalAccumulatedPoints[currentRound]; } + numRoundsClaimed[msg.sender] += roundLoop; if (claimableNRN > 0) { amountClaimed[msg.sender] += claimableNRN; _neuronInstance.mint(msg.sender, claimableNRN); emit Claimed(msg.sender, claimableNRN); } }
DoS
#0 - c4-pre-sort
2024-02-24T00:03:38Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-24T00:04:22Z
raymondfam marked the issue as duplicate of #1541
#2 - c4-judge
2024-03-11T13:01:33Z
HickupHH3 marked the issue as duplicate of #216
#3 - c4-judge
2024-03-11T13:08:05Z
HickupHH3 changed the severity to 2 (Med Risk)
#4 - c4-judge
2024-03-12T02:36:20Z
HickupHH3 marked the issue as partial-50
#5 - c4-judge
2024-03-21T02:58:59Z
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
FighterFarm.reRoll()
take fee and provide income for project. Any DNA manipulation to prevent further reroll is an issue.
NFT DNA reroll can be manipulated to get maximum attribute and rarity. Basically cheating to get best NFT for battle simulation
reroll DNA using msg.sender
,tokenId
,numRerolls
as hash
uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
All 3 components is predictable. NFT can be transfer away. So msg.sender
can be anything that user generate.
When user can generate their own DNA. Look at how rarity is generated. https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/AiArenaHelper.sol#L98-L119
They only need to find DNA that rarityPoint = (dna / [2,3,5,11,13] ) % 100
and still give high rarity. Give user best physical attributes for their NFT
Use tokenId
and numRerolls
only if do not want user to control reroll process.
uint256 dna = uint256(keccak256(abi.encode(tokenId, numRerolls[tokenId])));
Math
#0 - c4-pre-sort
2024-02-24T01:54:07Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-24T01:54:22Z
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:52:22Z
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:21:48Z
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.7567 USDC - $0.76
https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L493-L497 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L539-L545
FighterFarm.sol
NFT suppose to be transferable to another account if have no staked token.
But is not due to missing logic check in RankedBattle._addResultPoints()
.
NFT token no longer have staked token. But still cannot transfer to another account.
It require user to call unstake()
zero token is make it transferable
NFT cannot be transferred to another account if it is staked.
function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) { return ( _isApprovedOrOwner(msg.sender, tokenId) && balanceOf(to) < MAX_FIGHTERS_ALLOWED && !fighterStaked[tokenId] ); }
Stake NFT can only happen in RankedBattle.sol
https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L244-L290
Look at this logic. When token no longer have any stake, remove staking status
if (amountStaked[tokenId] == 0) { _fighterFarmInstance.updateFighterStaking(tokenId, false); }
RankedBattle.updateBattleRecord()
can remove all amountStaked[tokenId]
when fighter lost battle.
These stake token are moved to stake at risk.
But does not update staking status when it no longer have any stake.
bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk); if (success) { _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner); amountStaked[tokenId] -= curStakeAtRisk; //@curStakeAtRisk is clamped max to amountStaked[tokenId] //@fund is moved to stake at risk }
When round is over, admin can sweep all stake at risk to treasury.
Then token will no longer have any stake but staking status still persist.
This prevent NFT from transfer away unless call unstake()
first.
https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L493-L497 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L539-L545
Always update staking status.
/// If the user has stake-at-risk for their fighter, reclaim a portion /// Reclaiming stake-at-risk puts the NRN back into their staking pool if (curStakeAtRisk > 0) { + if(amountStaked[tokenId] == 0) { + _fighterFarmInstance.updateFighterStaking(tokenId, true); + } _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner); amountStaked[tokenId] += curStakeAtRisk; }
bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk); if (success) { _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner); amountStaked[tokenId] -= curStakeAtRisk; + if (amountStaked[tokenId] == 0) { + _fighterFarmInstance.updateFighterStaking(tokenId, false); + } }
Token-Transfer
#0 - c4-pre-sort
2024-02-24T05:06:29Z
raymondfam marked the issue as insufficient quality report
#1 - c4-pre-sort
2024-02-24T05:06:36Z
raymondfam marked the issue as primary issue
#2 - raymondfam
2024-02-24T05:07:04Z
Inconsistent and adequate proof when compared to #1641.
#3 - c4-judge
2024-03-13T10:11:36Z
HickupHH3 marked the issue as partial-75
#4 - c4-judge
2024-03-13T10:11:56Z
HickupHH3 marked the issue as duplicate of #1641
π 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
59.2337 USDC - $59.23
https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L345-L347 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L268-L276 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L253-L255 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L539-L545
User NFT fighter need 10 energy for one battle. The daily limit 100 energy. Then user need to pay 10_000 NRN for battery game item to recharge all energy.
Because NFT FighterFarm.sol
only prevent transfer in case of NFT already have NRN staked.
And RankedBattle.sol
allow user to fight with no money at stake.
Basically, user can transfer NFT to another address after 10 rounds and energy will be restore to full as energy storage unique to owner address not token.
Loss of income from battery sale for developer.
User with no stake can have infinite battle. If they decided to only attack user with staked NFT and they won. User with staked NFT will lose their stake despite battle count limitation through energy spending check.
function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) { return ( _isApprovedOrOwner(msg.sender, tokenId) && balanceOf(to) < MAX_FIGHTERS_ALLOWED && !fighterStaked[tokenId] ); }
function updateFighterStaking(uint256 tokenId, bool stakingStatus) external { require(hasStakerRole[msg.sender]); fighterStaked[tokenId] = stakingStatus; if (stakingStatus) { emit Locked(tokenId); } else { emit Unlocked(tokenId); } }
RankedBattle
only lock NFT transfer after stake token to NFT. LinkVoltageManager
refresh energy per address every 24 hours. LinkExploiting VoltageManager
energy refresh for all new address. By transferring NFT away after energy is depleted. Energy will complete recharge.
So user with no stake can freely battle without spending money buying battery pack to recharge.
It cost 10000 NRN token to buy battery. So user can avoid paying to play more than 10 battles per day.
https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L345-L347 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L268-L276 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L253-L255 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L539-L545
It make more sense to attach energy requirement spending/recharge to each NFT token instead of owner address.
Easiest fix, make NFT transfer also cost 1 energy voltage.
Token-Transfer
#0 - c4-pre-sort
2024-02-25T18:00:40Z
raymondfam marked the issue as insufficient quality report
#1 - c4-pre-sort
2024-02-25T18:00:59Z
raymondfam marked the issue as duplicate of #43
#2 - c4-judge
2024-03-07T06:26:59Z
HickupHH3 marked the issue as satisfactory