Handle
@GalloDaSballo
Vulnerability details
Impact
Detailed description of the impact of this finding.
randomIndex: https://github.com/code-423n4/2021-04-redacted/blob/2ec4ce8e98374be2048126485ad8ddacc2d36d2f/Beebots.sol#L307
Is not random
Any miner has access to these values
uint index = uint(keccak256(abi.encodePacked(nonce, msg.sender, block.difficulty, block.timestamp))) % totalSize;
Non miner attackers could also test the minting random condition until they get the id they are looking to access
https://medium.com/dedaub/bad-randomness-is-even-dicier-than-you-think-7fa2c6e0c2cd
The internal variable indices https://github.com/code-423n4/2021-04-redacted/blob/2ec4ce8e98374be2048126485ad8ddacc2d36d2f/Beebots.sol#L158
Seems to be used to avoid this type of collision
While this makes it less straightforward there is still the possibility of minting a token with a specific id.
That said, _addNFToken
https://github.com/code-423n4/2021-04-redacted/blob/2ec4ce8e98374be2048126485ad8ddacc2d36d2f/Beebots.sol#L408
is checking if the token is already owned by an address, ensuring a token can't be stolen.
Refactoring as suggested below will save gas, make code easier to read, and prevent reverts in rare unfortunate occasions of clashes
Proof of Concept
Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
See this article
https://medium.com/dedaub/bad-randomness-is-even-dicier-than-you-think-7fa2c6e0c2cd
Recommended Mitigation Steps
Refactor to not generate random Ids, instead use counters, it will make the code more predictable and easier to read, avoids clashing of ids, reduces the need to track minted tokens.
See and example by Austin Griffith: https://github.com/austintgriffith/scaffold-eth/blob/buyer-mints-nft/packages/hardhat/contracts/YourCollectible.sol
If you use counters, you can get rid of the entire random generation, including the variables used for it:
https://github.com/code-423n4/2021-04-redacted/blob/2ec4ce8e98374be2048126485ad8ddacc2d36d2f/Beebots.sol#L156
This will also make calculating totalSize by just looking at counters:
https://github.com/code-423n4/2021-04-redacted/blob/2ec4ce8e98374be2048126485ad8ddacc2d36d2f/Beebots.sol#L308
Handle
@GalloDaSballo
Vulnerability details
Impact
Detailed description of the impact of this finding.
randomIndex: https://github.com/code-423n4/2021-04-redacted/blob/2ec4ce8e98374be2048126485ad8ddacc2d36d2f/Beebots.sol#L307
Is not random
Any miner has access to these values
uint index = uint(keccak256(abi.encodePacked(nonce, msg.sender, block.difficulty, block.timestamp))) % totalSize;
Non miner attackers could also test the minting random condition until they get the id they are looking to access
https://medium.com/dedaub/bad-randomness-is-even-dicier-than-you-think-7fa2c6e0c2cd
The internal variable
indiceshttps://github.com/code-423n4/2021-04-redacted/blob/2ec4ce8e98374be2048126485ad8ddacc2d36d2f/Beebots.sol#L158Seems to be used to avoid this type of collision
While this makes it less straightforward there is still the possibility of minting a token with a specific id.
That said, _addNFToken
https://github.com/code-423n4/2021-04-redacted/blob/2ec4ce8e98374be2048126485ad8ddacc2d36d2f/Beebots.sol#L408
is checking if the token is already owned by an address, ensuring a token can't be stolen.
Refactoring as suggested below will save gas, make code easier to read, and prevent reverts in rare unfortunate occasions of clashes
Proof of Concept
Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
See this article
https://medium.com/dedaub/bad-randomness-is-even-dicier-than-you-think-7fa2c6e0c2cd
Recommended Mitigation Steps
Refactor to not generate random Ids, instead use counters, it will make the code more predictable and easier to read, avoids clashing of ids, reduces the need to track minted tokens.
See and example by Austin Griffith: https://github.com/austintgriffith/scaffold-eth/blob/buyer-mints-nft/packages/hardhat/contracts/YourCollectible.sol
If you use counters, you can get rid of the entire random generation, including the variables used for it:
https://github.com/code-423n4/2021-04-redacted/blob/2ec4ce8e98374be2048126485ad8ddacc2d36d2f/Beebots.sol#L156
This will also make calculating totalSize by just looking at counters:
https://github.com/code-423n4/2021-04-redacted/blob/2ec4ce8e98374be2048126485ad8ddacc2d36d2f/Beebots.sol#L308