search

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

1 stars 0 forks source link

`getRandomTokenIdFromFund` yields wrong probabilities for ERC1155 #56

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

@cmichelio

Vulnerability details

Vulnerability Details

NFTXVaultUpgradeable.getRandomTokenIdFromFund does not work with ERC1155 as it does not take the deposited quantity1155 into account.

Impact

Assume tokenId0 has a count of 100, and tokenId1 has a count of 1. Then getRandomId 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.

cemozerr commented 3 years ago

Marking this as high risk as an attacker can weed out high-value NFTs from a vault putting other users funds at risk

0xKiwi commented 3 years ago

Hello. The rationale for assigning this as a high risk issue doesn't make much sense. If NFTs are in a pool its because they are all the same value. If anyone wants a rarer they can either just game it or perform a target redeem.