[M-05/#578] Mitigation error: user influence on DNA via `to`

[c4-bot-8]

Lines of code

https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/blob/d81beee0df9c5465fe3ae954ce41300a9dd60b7f/src/FighterFarm.sol#L366

Vulnerability details

[M-05/#578] Mitigation error - introduction of issue 2. in mintFromMergingPool()

This is not really an error from the mitigation of M-05, but from the mitigation of #578, which was a quite distinct (low severity) issue.

Impact

The user can transfer his token to an address generated by brute force to get a DNA set in mintFromMergingPool() such that it mints a fighter with better, or even optimal attributes. The impact is parallel to the one on reRoll() and claimFighters() (issue 2. in my mitigation review of M-05), but now impacting mintFromMergingPool().

Proof of Concept

In the mitigation of #578, the DNA in mintFromMergingPool() was changed from uint256(keccak256(abi.encode(msg.sender, fighters.length))) to uint256(keccak256(abi.encode(to, fighters.length))). 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.

To change the address the user has to unstake (so transfers are allowed) and transfer his token to the new address before the admin calls pickWinners(). This can be done preemptively (if frontrunning is infeasible), simply predicting he may win, and in that case get the desired DNA.

Another complication is that fighters.length also determines the DNA. It is not necessary at all that this will change in between his fighter transfer and the admin's call to pickWinners(), even if the user cannot frontrun the admin. And even if it does change, the user can with near certainty constrain an estimate to within a few values. He can then optimize his new address with all these values taken into account. As long as they are few he can still ensure a good, or even optimal DNA. (He just needs to optimize over a random tuple instead of a random scalar.)

Recommended Mitigation Steps

Simply removing to, i.e. set DNA to uint256(keccak256(abi.encode(fighters.length))), precludes the user from influencing the DNA to the extent described here. But then he may still wait for a better fighters.length (issue 3. in my mitigation review of M-05), which will also enable him to know in advance all the DNAs (like in issue 4. in my mitigation review of M-05). Adding blockhash(block.number - 1) mitigates this up to the issue actually reported in M-05, i.e. that the user can retry claimRewards() by reverting any undesirable fighter (see my mitigation review of M-05 for details). But in this case we can make use of the fact that this process involves two transactions. If the DNA is set in pickWinners() using blockhash(block.number - 1), which is called by the admin, then the user cannot influence this.

Assessed type

Other

[jhsagd76]

IMO, its the same issue with same root cause in the #68

[c4-judge]

jhsagd76 marked the issue as satisfactory

[c4-judge]

jhsagd76 marked the issue as duplicate of #68

[c4-judge]

jhsagd76 changed the severity to 2 (Med Risk)

[liveactionllama]

Per request from the judge @jhsagd76 here, updating the labels on this issue accordingly.