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
[P1-L08] Add input validation throughout DVM #1212
Conversation
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Two minor nits.
The coverage failure is very worrying. This is an issue we've seen in the past (before we bumped solidity coverage to 0.7.0). I really hope it hasn't come back. Gonna pull your branch down and do a quick investigation. |
After more investigation, the problem has little to do with solidity coverage. It's a solc thing. Somehow, this completely reasonable PR makes our code fail to compile without the optimizer enabled. |
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
@@ -137,6 +138,7 @@ abstract contract MultiRole { | |||
* managing role for `roleId`. | |||
*/ | |||
function addMember(uint roleId, address newMember) public onlyShared(roleId) onlyRoleManager(roleId) { | |||
require(newMember != address(0x0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This addMember
function within the Role
lib was not specifically pointed out during the audit and this function follows a different pattern: it’s public and has different privileges on who can call it. I added the same require to keep things consistent. let me know if you have any issues with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just calls the other addMember
function on the next line, so I think this has the effect of doing this check twice. I would suggest removing this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome, a few quick comments.
@@ -11,7 +11,7 @@ library Exclusive { | |||
} | |||
|
|||
function resetMember(RoleMembership storage roleMembership, address newMember) internal { | |||
require(newMember != address(0x0), "Cannot set an exclusive role to 0x0"); | |||
require(newMember != address(0x0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we need to remove this pre-existing require message? I don't think this one breaks the non optimized compilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it to be consistent with the other require messages on these functions
@@ -137,6 +138,7 @@ abstract contract MultiRole { | |||
* managing role for `roleId`. | |||
*/ | |||
function addMember(uint roleId, address newMember) public onlyShared(roleId) onlyRoleManager(roleId) { | |||
require(newMember != address(0x0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just calls the other addMember
function on the next line, so I think this has the effect of doing this check twice. I would suggest removing this one.
@@ -35,6 +35,7 @@ library Shared { | |||
} | |||
|
|||
function addMember(RoleMembership storage roleMembership, address memberToAdd) internal { | |||
require(memberToAdd != address(0x0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this the only require without a message and add a comment above it pointing to the issue and commenting the require message that we'd like to have here once the compilation issue is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, sorry, is this the one that breaks it? If not, can you add an error message back and move the above comment to the one that actually breaks the compilation?
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Finally have CI passing this. Let me know @mrice32 @ptare @nicholaspai if you guys are happy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I may have gotten confused on my last round of comments, but shouldn't there be one require
with no error message (the one in VoteTiming, perhaps)?
@@ -35,6 +35,7 @@ library Shared { | |||
} | |||
|
|||
function addMember(RoleMembership storage roleMembership, address memberToAdd) internal { | |||
require(memberToAdd != address(0x0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, sorry, is this the one that breaks it? If not, can you add an error message back and move the above comment to the one that actually breaks the compilation?
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Ye, sorry. I'm with you now. I added back all the requires except the one in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Added a suggestion to fix a typo.
Co-Authored-By: Matt Rice <matthewcrice32@gmail.com>
There are a number of places in the DVM that do not have sufficient input validation. This PR adds this as well as associated tests.