Additionally, a mitigation of #578, which was duplicated to M-05 (#376), has been attempted. This issue was about the DNA simply being differently generated, due to a likely mistake, but with low impact.
M-05: Can mint NFT with the desired attributes by reverting transaction is only 1. above, and I consider the other issues (including #578) to have been incorrectly or misleadingly duplicated to M-05, and I have to deal with them as separate/new issues/errors.
I have submitted reports on 2-4 as three new issues, as well as a report on the error from the mitigation of #578. I will make use of this submission as the report of an unmitigated 1., the details on which and recommended mitigation steps follow at the end of this report, after an overview of the review of these issues and the error.
The mitigation review of the part of the scope which is "M-05" thus consists of a total of 5 reports:
This report.
"User influence on DNA in claimFighters() (unmitigated issue 2. grouped under M-05)"
"User influence on DNA via fighters.length in mintFromMergingPool() (unmitigated issue 3. grouped under M-05)"
"The user can select the best out of all rerolls (unmitigated issue 4. grouped under M-05)"
Error. "[M-05/#578] Mitigation error: user influence on DNA via to"
Nothing has been done about this in mintFromMergingPool().
In claimFighters()fighters.length has been replaced by nftsClaimed[msg.sender][0], nftsClaimed[msg.sender][1]. This is only slightly better. fighters.length will be updated by all other users so the attacker would get a steady stream of new values. nftsClaimed[msg.sender][] is changed only by the user. However, by transferring the token to another address this value will change, so the user can still influence the DNA.
Nothing has been done to mitigate this.
In summary, none of the issues 1-4 have been fully mitigated.
578 lead to replacing msg.sender by to. This introduced the error discussed below, also reported separately.
Mitigation error - introduction of issue 2. in mintFromMergingPool()
In the mitigation of #578, having changed the DNA in mintFromMergingPool() from uint256(keccak256(abi.encode(msg.sender, fighters.length))) to uint256(keccak256(abi.encode(to, fighters.length))) has introduced issue 2. for this function as well. to is set to msg.sender from MergingPool.claimRewards(). This is checked to be a winner, which is set by the admin in pickWinners() as the owner of the token. The user can transfer his token to an appropriate address, such that if he is picked as the winner this address generates the desired DNA. A complication for the user is that fighters.length might change, so he needs to take the few possible such values into account, and try to limit them by acting quickly and hoping not many fighters will be minted in between. In any case he can significantly influence his DNA.
Overview of mitigation steps for 1-4 and the error
Do not set the DNA in the same transaction as the user receives the fighter.
Do not use msg.sender to set the DNA.
Do not use fighters.length to set the DNA.
Do not use currently known values to set the DNA.
Error: Do not use to (here equivalent to msg.sender in 2.) to set the DNA.
If a third party decentralized VRF is not an option, you can act as your own centralized randomness provider, otherwise equivalent to the typical VRF oracles. The only consideration then is whether users will trust that your server is unbiased and uninfluenceable.
Otherwise it seems the best solution is to set the DNA as blockhash(block.number - 1) in a call from the admin, and then in a second call by the user assign the fighter to the user, whether this is by direct minting to the user, by transferring the fighter from an escrow (e.g. FighterFarm itself), or as a finalizing of the reroll.
M-05: Can mint NFT with the desired attributes by reverting transaction
To clarify the original report:
This issue makes no claim about the direct influence of the DNA calculated in _createNewFighter(). The DNA is set in the same transaction as the the fighter is minted. The issue is that the user can let the DNA be calculated and the fighter minted to him, and then, based on what he is to receive, revert the transaction. This is enabled by the onERC721Received() hook.
_createNewFighter() calls _safeMint(to, newId). This calls ERC721Utils.checkOnERC721Received(_msgSender(), address(0), to, newId, ""), which on a contract executes
The user (his contract) may thus implement his onERC721Received() to reject unwanted fighters, e.g.
function onERC721Received(
address operator,
address from,
uint256 tokenId,
bytes calldata data
) public returns (bytes4){
(,,uint256 weight,,,,) = fighterFarm.getAllFighterInfo(tokenId);
require(weight == 95, "I don't want this attribute");
return bytes4(keccak256(bytes("onERC721Received(address,address,uint256,bytes)")));
}
Recommended mitigation steps
There is no way to avoid this issue if the randomness is set in the same transaction as the minting/onERC721Received() call. The DNA must be committed first, and then, in a separate transaction, should the user be able to get his fighter. One way is to mint the fighter to the FighterFarm contract, acting as an escrow, and implementing a function for the user to claim it by having it transferred to him.
Lines of code
Vulnerability details
Mitigation of M-05: NOT mitigated, with error, see comments
https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/blob/d81beee0df9c5465fe3ae954ce41300a9dd60b7f/src/FighterFarm.sol#L574
Mitigated issue(s)
M-05: Can mint NFT with the desired attributes by reverting transaction (Also see duplicated reports #1017, #578, #1991)
There were four issues with the randomness of the DNA in FighterFarm.sol:
_createNewFighter()
by reverting theonERC721Received()
hook.msg.sender
which seeds the DNA inreRoll()
andclaimFighters()
.fighters.length
which seeds the DNA inmintFromMergingPool()
andclaimFighters()
.(The issue of freely setting DNA in
mintFromMergingPool()
is part of H-03: Players have complete freedom to customize the fighter NFT when calling redeemMintPass and can redeem fighters of types Dendroid and with rare attributes.)Additionally, a mitigation of #578, which was duplicated to M-05 (#376), has been attempted. This issue was about the DNA simply being differently generated, due to a likely mistake, but with low impact.
M-05: Can mint NFT with the desired attributes by reverting transaction is only 1. above, and I consider the other issues (including #578) to have been incorrectly or misleadingly duplicated to M-05, and I have to deal with them as separate/new issues/errors. I have submitted reports on 2-4 as three new issues, as well as a report on the error from the mitigation of #578. I will make use of this submission as the report of an unmitigated 1., the details on which and recommended mitigation steps follow at the end of this report, after an overview of the review of these issues and the error. The mitigation review of the part of the scope which is "M-05" thus consists of a total of 5 reports:
claimFighters()
(unmitigated issue 2. grouped under M-05)"fighters.length
inmintFromMergingPool()
(unmitigated issue 3. grouped under M-05)""The user can select the best out of all rerolls (unmitigated issue 4. grouped under M-05)"
Error. "[M-05/#578] Mitigation error: user influence on DNA via
to
"Mitigation re- and overview
Nothing has been done to mitigate this.
has been mitigated in
reRoll()
by replacingmsg.sender
withtokenId
. But it has NOT been mitigated inclaimFighters()
.Nothing has been done about this in
mintFromMergingPool()
. InclaimFighters()
fighters.length
has been replaced bynftsClaimed[msg.sender][0], nftsClaimed[msg.sender][1]
. This is only slightly better.fighters.length
will be updated by all other users so the attacker would get a steady stream of new values.nftsClaimed[msg.sender][]
is changed only by the user. However, by transferring the token to another address this value will change, so the user can still influence the DNA.Nothing has been done to mitigate this.
In summary, none of the issues 1-4 have been fully mitigated.
578 lead to replacing
msg.sender
byto
. This introduced the error discussed below, also reported separately.Mitigation error - introduction of issue 2. in
mintFromMergingPool()
In the mitigation of #578, having changed the DNA in
mintFromMergingPool()
fromuint256(keccak256(abi.encode(msg.sender, fighters.length)))
touint256(keccak256(abi.encode(to, fighters.length)))
has introduced issue 2. for this function as well.to
is set tomsg.sender
fromMergingPool.claimRewards()
. This is checked to be a winner, which is set by the admin inpickWinners()
as the owner of the token. The user can transfer his token to an appropriate address, such that if he is picked as the winner this address generates the desired DNA. A complication for the user is thatfighters.length
might change, so he needs to take the few possible such values into account, and try to limit them by acting quickly and hoping not many fighters will be minted in between. In any case he can significantly influence his DNA.Overview of mitigation steps for 1-4 and the error
msg.sender
to set the DNA.fighters.length
to set the DNA.Do not use currently known values to set the DNA.
Error: Do not use
to
(here equivalent tomsg.sender
in 2.) to set the DNA.If a third party decentralized VRF is not an option, you can act as your own centralized randomness provider, otherwise equivalent to the typical VRF oracles. The only consideration then is whether users will trust that your server is unbiased and uninfluenceable. Otherwise it seems the best solution is to set the DNA as
blockhash(block.number - 1)
in a call from the admin, and then in a second call by the user assign the fighter to the user, whether this is by direct minting to the user, by transferring the fighter from an escrow (e.g. FighterFarm itself), or as a finalizing of the reroll.M-05: Can mint NFT with the desired attributes by reverting transaction
To clarify the original report: This issue makes no claim about the direct influence of the DNA calculated in
_createNewFighter()
. The DNA is set in the same transaction as the the fighter is minted. The issue is that the user can let the DNA be calculated and the fighter minted to him, and then, based on what he is to receive, revert the transaction. This is enabled by theonERC721Received()
hook._createNewFighter()
calls_safeMint(to, newId)
. This callsERC721Utils.checkOnERC721Received(_msgSender(), address(0), to, newId, "")
, which on a contract executesThe user (his contract) may thus implement his
onERC721Received()
to reject unwanted fighters, e.g.Recommended mitigation steps
There is no way to avoid this issue if the randomness is set in the same transaction as the minting/
onERC721Received()
call. The DNA must be committed first, and then, in a separate transaction, should the user be able to get his fighter. One way is to mint the fighter to the FighterFarm contract, acting as an escrow, and implementing a function for the user to claim it by having it transferred to him.