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

ConversionHelpers: check length on bytes to uint256[] #509

Merged
merged 1 commit into from Apr 14, 2019

Conversation

Projects
None yet
4 participants
@sohkai
Copy link
Member

sohkai commented Apr 12, 2019

There are publicly exposed interfaces that expect bytes and immediately turn them into uint256[] (e.g. hasPermission() in the ACL and Kernel.

There might be some cases where the truncation could lead to Bad ThingsTM, like the ACL being tricked into thinking a contract had permission to do something when it actually didn't. We never use the bytes form of hasPermission() directly ourselves, so this isn't exploitable, but could be if an external contract decided to.

With help from @wadeAlexC.

Bytecode diff (AragonApp doesn't use dangerouslyCastBytesToUintArray()):

                           CODE DEPOSIT COST    DEPLOYED BYTES     INITIALIZATION BYTES
ACL.json                   33200 more gas       +166               0
TestACLInterpreter.json    33200 more gas       +166               0
TestConversionHelpers.json 707400 more gas      +3537              0
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 12, 2019

Coverage Status

Coverage increased (+0.002%) to 99.542% when pulling efe170c on conversion-helpers-check-bytes-length into 28ad69d on dev.

@facuspagnuolo
Copy link
Contributor

facuspagnuolo left a comment

LGTM!

@izqui

izqui approved these changes Apr 12, 2019

@sohkai sohkai merged commit b6823b5 into dev Apr 14, 2019

5 checks passed

License Compliance All checks passed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 99.542%
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the conversion-helpers-check-bytes-length branch Apr 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.