Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vulnerability: Nonce must be limited to 2^127 in split bit design #36

Open
CryptoKiddies opened this issue Jan 21, 2022 · 1 comment
Open

Comments

@CryptoKiddies
Copy link

CryptoKiddies commented Jan 21, 2022

Preface
In practice, achieving an index value greater than 2 ** 127 + 1 is infeasible by minting in unit increments, so deployment of this contract as is should be safe. The suggestion below is mostly for logical soundness, however users who make modifications to the prototype by replacing the base type incrementer with custom id's should be made aware, and perhaps guards should be put in place.

Expected Behavior

In contract {ERC1155MixedFungibleMintable}, all _type values in the range supported by its integer type uint256 can be created and accessed without clashes and access control insecurity.

Actual Behavior

The current demonstration code runs the risk of calls reduced to the same base _type for the index range between 2**127 and 2**256 - 1, where in a simplified 8-bit demonstration of left-shifted index values masked by the TYPE_NF_MASK:

0001 0001 << 4 | 1000 0000 and 0000 0001 << 4 | 1000 0000

result in the sender address being overwritten in the create() call on L137 of ERC1155MixedFungibleMintable

In addition, any nonce value where the lower half order of bits falls between xxxx 1000 and xxxx 1111 will clash with values ranging in xxxx 0000 and xxxx 0111.

As such, regarding these two attack vectors, the nonce value should not exceed 2 ** (256/2 - 1) - 1.

Similarly, maxIndex should not exceed 2 ** (128) - 1.

Suggestion

Limit _type to a supported uint type less than 2 ** 127, i.e. uint120 where compiler ^0.8 can handle overflow checks, or add validation logic to revert index values greater than that safe value, or utilize recent feature of User-Defined Types. A more expensive and unnecessarily complicated construction would be to create and manipulate 2 word custom integers, e.g., a Uint512Lib custom library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants
@CryptoKiddies and others