search

code-423n4 / 2021-12-nftx-findings

0 stars 0 forks source link

`getRandomTokenIdFromVault` yields wrong probabilities for ERC1155 #148

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

cmichel

Vulnerability details

NFTXVaultUpgradeable.getRandomTokenIdFromVault does not work well with ERC1155 as it does not take the deposited quantity1155 into account. The randomness is just across holding buckets

POC

Assume the contract has 100 tokenId = 0 deposited, but only 1 tokenId = 1. Then getRandomTokenIdFromVault would have a pseudo-random 1:1 chance for token 0 and 1 when in reality it should be 100:1.

This might make it easier for an attacker to redeem more valuable NFTs as the probabilities are off.

Recommended Mitigation Steps

Take the quantities of each token into account (quantity1155) which probably requires a design change as it's currently hard to do without iterating over all tokens.

0xKiwi commented 2 years ago

This is a valid report, but as mentioned in the review is difficult to solve, and we don't care for the onchain pseudorandomness very much. Acknowledging and disagreeing with severity (why is this high risk?)

dmvt commented 2 years ago

This is at worst a state handling issue. Every NFT in the bucket is assumed to be equivalent. At worst the user gets a specific NFT at the random fee level. It seems like a stretch that the user would be intelligent enough and spend the time to find this 1 in a million use case and exploit it to save 4% or whatever the fee difference is.

dmvt commented 2 years ago

It seems that in #149 the warden was aware this is out of scope. The sponsor is being nice by even acknowledging the issue. I'm going to invalidate this one too.