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: 180/283
Findings: 6
Award: $6.28
๐ 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
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L363-L364 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L346-L347
Incomplete implementation of ERC721 allows users to bypass 'MAX_FIGHTERS_ALLOWED' and send already staked fighters due to lack of overriding.
There are several public transfer functions in ERC721 standard.
/** * @dev See {IERC721-transferFrom}. */ function transferFrom( address from, address to, uint256 tokenId ) public virtual override { //solhint-disable-next-line max-line-length require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner nor approved"); _transfer(from, to, tokenId); } /** * @dev See {IERC721-safeTransferFrom}. */ function safeTransferFrom( address from, address to, uint256 tokenId ) public virtual override { safeTransferFrom(from, to, tokenId, ""); } /** * @dev See {IERC721-safeTransferFrom}. */ function safeTransferFrom( address from, address to, uint256 tokenId, bytes memory data ) public virtual override { require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner nor approved"); _safeTransfer(from, to, tokenId, data); }
However, the FighterFarm
contract does not override safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data)
, thus having the original function of ERC721.
Users can transfer already staked fighters and receive fighters even if the balance has already reached MAX_FIGHTERS_ALLOWED
by calling directly the safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data)
function to bypass the _ableToTransfer
function.
Manual Review
It is recommended to implement safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data)
.
ERC721
#0 - c4-pre-sort
2024-02-23T05:15:29Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-23T05:15:40Z
raymondfam marked the issue as duplicate of #739
#2 - c4-judge
2024-03-11T02:09:26Z
HickupHH3 changed the severity to 3 (High Risk)
#3 - c4-judge
2024-03-11T02:46:10Z
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
https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol#L301-L302
Locked game items can be sent bypassing check by calling directly safeBatchTransferFrom
function.
If a game item is marked as locked (e.g. transferable
is false
), the GameItems
contract will not allow the user to send the item.
GameItems
is an ERC1155 token contract that allows users to transfer tokens in batches.
However it overrides safeTransferFrom
but not safeBatchTransferFrom
.
So user can bypass the check whether is locked to transfer, and send locked token to others.
Manual review
It is recommend that safeBatchTransferFrom
should be also implemented to check the locked state.
Token-Transfer
#0 - c4-pre-sort
2024-02-22T04:10:16Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-22T04:10:22Z
raymondfam marked the issue as duplicate of #18
#2 - c4-pre-sort
2024-02-26T00:28:53Z
raymondfam marked the issue as duplicate of #575
#3 - c4-judge
2024-03-05T04:47:38Z
HickupHH3 changed the severity to 3 (High Risk)
#4 - c4-judge
2024-03-05T04:55:33Z
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#L380-L389
Because fighterType
is used to reroll
fighters, there is no clear distinction between champions and dendroids.
The user can rerolls a fighter created as a champion by dendroid fighterType
, later but that fighter has all the features of a dendroid and keeps dendroidBool as false.
Rerolls modify the features like element, weight, generation, dna, but physical attributes uses original dendroidBool
even though fighterType
is different from original.
Manual review
It is recommended to allow fighter keeps his fighterType
or update dendroidBool
by fighterType
while rerolling.
Error
#0 - c4-pre-sort
2024-02-22T01:44:38Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-22T01:45:32Z
raymondfam marked the issue as duplicate of #306
#2 - c4-judge
2024-03-05T04:33:43Z
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/main/src/RankedBattle.sol#L527-L532 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L445
When calculating stakeFactor
, we divide by 10 ^ 18 to ignore the effect of all parts of the token that are less than 1 ether (10 ^ 18), then use the sqrt function from the FixdPointMathLib library.
Also, if the calculation result is 0, stakeFactor becomes 1.
It can cause losing a lot in points calculations and allows user to get good results with very little money.
The formula for calculation of staking factor is like the below.
function _getStakingFactor( uint256 tokenId, uint256 stakeAtRisk ) private view returns (uint256) { // @audit effect is same 1 wei == 1.99 L527: uint256 stakingFactor_ = FixedPointMathLib.sqrt( (amountStaked[tokenId] + stakeAtRisk) / 10**18 ); if (stakingFactor_ == 0) { stakingFactor_ = 1; } return stakingFactor_; }
Let's assume 'amountStaked + SteakAtRisk' is 'totalStaked'.
If totalStaked
is 1 wei, returns 1
If totalStaked
is 4 * 10 ** 18 - 1 (about 4 ethers), the result is the same.
So the result for the (0, 4 ether) region of totalStaked
is 1.
And the result for the [4, 9 ether) area of totalStaked
is 2.
...
And as the amount increases, the effect of larger portions disappears.
For example, if a user wants to stake 8 ethers on his fighter, it is better to stake only 4 as the results will be the same.
Manual review
function _getStakingFactor( uint256 tokenId, uint256 stakeAtRisk ) private view returns (uint256) { // @audit result is same 1 wei == 3.9999 ether uint256 stakingFactor_ = FixedPointMathLib.sqrt( - (amountStaked[tokenId] + stakeAtRisk) / 10**18 + (amountStaked[tokenId] + stakeAtRisk) * 10**18 ); if (stakingFactor_ == 0) { stakingFactor_ = 1; } return stakingFactor_; }
/// L427 - points = stakingFactor[tokenId] * eloFactor; + points = (stakingFactor[tokenId] * eloFactor) / 10 ** 18;
Or use points as 18 decimals instead.
Math
#0 - c4-pre-sort
2024-02-24T08:14:54Z
raymondfam marked the issue as insufficient quality report
#1 - c4-pre-sort
2024-02-24T08:15:02Z
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:46:43Z
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/main/src/FighterFarm.sol#L470 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L110
Creating fighters can not be working due to division by zero after generation is incremented. So the users won't be able to create and reroll the fighters after that.
numElements
is only set for the first generation in the FighterFarm
contract constructor on the deployment, the contract will not update it later in any case, so the value for other generations will be 0.
In fact, there is only one line in the FirghterFarm
contract that writes numElements
storage slots..
L85: mapping(uint8 => uint8) public numElements; constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_) ERC721("AI Arena Fighter", "FTR") { _ownerAddress = ownerAddress; _delegatedAddress = delegatedAddress; treasuryAddress = treasuryAddress_; L110: numElements[0] = 3; }
When creating new fighter by calling claimFighters
, redeemMintpass
, and rerolling fighter by calling reRoll
function, _createFighterBase
function will be called and it uses numElements
to determine element paramenter.
function _createFighterBase( uint256 dna, uint8 fighterType ) private view returns (uint256, uint256, uint256) { L470: uint256 element = dna % numElements[generation[fighterType]]; uint256 weight = dna % 31 + 65; uint256 newDna = fighterType == 0 ? dna : uint256(fighterType); return (element, weight, newDna); }
Therefore if generation is incremented, all transactions related to create fighter using _createFighterBase
will be reverted to division by 0.
Manual review
numElements
should be set as none zero value in increment of generation.
function incrementGeneration(uint8 fighterType) external returns (uint8) { require(msg.sender == _ownerAddress); generation[fighterType] += 1; maxRerollsAllowed[fighterType] += 1; numElements[generation[fighterType]] = 3; // or any non-zero value return generation[fighterType]; }
Other
#0 - c4-pre-sort
2024-02-22T18:42:16Z
raymondfam marked the issue as sufficient quality report
#1 - c4-pre-sort
2024-02-22T18:42:25Z
raymondfam marked the issue as duplicate of #45
#2 - c4-judge
2024-03-07T07:03:28Z
HickupHH3 marked the issue as satisfactory
๐ 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/MergingPool.sol#L154-L159 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L504
Lack of validation for customAttributes
may result in invalid fighters being lighter or heavier than those provided by the protocol.
The range for weights is 65-95, and each of the groups you indicated increase in weight for increments of 10.
A fighterโs weight is the primary determinant of its other relative strength and weaknesses according to documentation.
However, the claimRewards
function in the MergingPool
contract does not validate customAttributes for weights and as well as the _createNewFighter
function in the FighterFarm
contract.
MergingPool
contract doesn't check customAttributes
at all.
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); } }
FighterFarm contract set weight as customAttributes[1].
element = customAttributes[0]; weight = customAttributes[1];
Therefore claimable account can create invalid fighter that is not in the valid group by using customAttributes
[1] when claiming rewards.
This could result in lighter or heavier fighter planes that attackers want to build.
Manual review
Validation for customAttributes
in the FighterFarm
contract should be performed as follows:
/// L504: - weight = customAttributes[1]; + uint256 _weight = customAttributes[1]; + require(_weight >= 65 && weight <= 95, "fighter's weight is not in valid range"); + weight = _weight;
Invalid Validation
#0 - c4-pre-sort
2024-02-24T09:01:02Z
raymondfam marked the issue as insufficient quality report
#1 - c4-pre-sort
2024-02-24T09:01:13Z
raymondfam marked the issue as duplicate of #226
#2 - c4-judge
2024-03-11T10:25:22Z
HickupHH3 marked the issue as satisfactory