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

Add strict overflow checks for uint and int types #237

Closed

Conversation

albrow
Copy link

@albrow albrow commented Jul 23, 2018

This PR causes coderNumber to throw an error when encoding uint or int values that are too big too big to fit in the allotted size. For example, the max value for a number of type uint8 is 255. Now, if you try to encode an number greater than 255, you will see an error.

Prior to this PR, the behavior of coderNumber was to add additional bytes to the encoded value, (e.g., a uint256 value bigger than 2^256-1 would encode to 64 bytes instead of 32). This would result in an invalid encoding since int and uint types have specifically defined sizes in Solidity.

Edit: Prior to this PR, the behavior of coderNumber was to implicitly apply modulo 2^(size*8). So a uint256 value of 2^256 would encode to 0. This can lead to confusing and surprising behavior.

@ricmoo
Copy link
Member

ricmoo commented Jul 23, 2018

Ah! Good catch. I will look into this soon, any incorporate it into the v4 branch too.

This can likely be done more efficiently using the maskn operation too, but I will try out your test cases and get something out soon.

@ricmoo
Copy link
Member

ricmoo commented Jul 26, 2018

The v4 branch has fixes in place to do bounds checking on fixed bytes and on numbers. The v3 branch is coming.

@ricmoo
Copy link
Member

ricmoo commented Jul 26, 2018

The v4 branch has fixes that prevent out-of-range number and fixed-length bytes that are not exactly equal. The v3 branch only has overflow protection for fixed-length bytes. Fixing both for v3 could possibly be a breaking change, but since v4 is a major version change (that is still in a beta branch) it is safe to add breaking changes.

Once the Travis CI passes, I will publish 3.0.26 to npm. The 4.0.0-beta.5 on npm already includes these changes.

@ricmoo
Copy link
Member

ricmoo commented Jul 27, 2018

Published! :)

Closing this issue. If you have any further issues, please feel free to re-open.

Thanks! :)

@ricmoo ricmoo closed this Jul 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants