The XDEFIDistribution.merge function burns tokens, which decreases the ERC721Enumerable.totalSupply() and the _generateNewTokenId function returns a token ID as the concatenation of the points and totalSupply() + 1:
function _generateNewTokenId(uint256 points_) internal view returns (uint256 tokenId_) {
// Points is capped at 128 bits (max supply of XDEFI for 10 years locked), total supply of NFTs is capped at 128 bits.
return (points_ << uint256(128)) + uint128(totalSupply() + 1);
}
If a user is unlucky it can happen that there is a collision and they cannot merge their tokens as the _safeMint fails when trying to mint an already existing token ID.
POC
user A has two token IDs with points 100 and 200
Imagine the current totalSupply() = N and the second last token that was minted has 300 points. Its token id is: tokenId = (N - 1) << 128 | 300
user A now tries to merge their tokens. The two burns decrease the total supply to N - 2. The mint uses totalSupply() + 1 = N - 2 + 1 = N - 1. The merged token ID is now also tokenId = (N-1) << 128 | 300. The merge fails as in ERC721._mint as this token already exists
Impact
Token merges can fail
Recommended Mitigation Steps
You probably don't want totalSupply() to ever decrease.
Either don't call _burn and use a different way of invalidating these tokens.
Or just use a counter for the lower bits of tokenId instead of totalSupply() + 1.
deluca-mike
commented
2 years ago
Handle
cmichel
Vulnerability details
The
XDEFIDistribution.mergefunction burns tokens, which decreases theERC721Enumerable.totalSupply()and the_generateNewTokenIdfunction returns a token ID as the concatenation of the points and totalSupply() + 1:If a user is unlucky it can happen that there is a collision and they cannot merge their tokens as the
_safeMintfails when trying to mint an already existing token ID.POC
totalSupply() = Nand the second last token that was minted has 300 points. Its token id is:tokenId = (N - 1) << 128 | 300N - 2. The mint usestotalSupply() + 1 = N - 2 + 1 = N - 1. The merged token ID is now alsotokenId = (N-1) << 128 | 300. The merge fails as inERC721._mintas this token already existsImpact
Token merges can fail
Recommended Mitigation Steps
You probably don't want
totalSupply()to ever decrease. Either don't call_burnand use a different way of invalidating these tokens. Or just use a counter for the lower bits oftokenIdinstead oftotalSupply() + 1.