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

Payroll: Hardcode role constant values #899

Merged
merged 1 commit into from Jul 3, 2019

Conversation

Projects
None yet
3 participants
@facuspagnuolo
Copy link
Contributor

commented Jul 3, 2019

Fixing finding 6.9 of the audit report

6.9 Payroll - Internal role constants can be slightly gas optimized

Severity Status Remediation Comment
Minor Open This issue is currently under review.

Description

A slight gas optimization can be done to the *_ROLE constants.

At the moment, the code is very readable because the constants are clearly specified and the developer and reviewer understand and trust the value computed is the correct one.

code/payroll/future-apps/payroll/contracts/Payroll.sol:L23-L30

bytes32 constant public ADD_EMPLOYEE_ROLE = keccak256("ADD_EMPLOYEE_ROLE");
bytes32 constant public TERMINATE_EMPLOYEE_ROLE = keccak256("TERMINATE_EMPLOYEE_ROLE");
bytes32 constant public SET_EMPLOYEE_SALARY_ROLE = keccak256("SET_EMPLOYEE_SALARY_ROLE");
bytes32 constant public ADD_BONUS_ROLE = keccak256("ADD_BONUS_ROLE");
bytes32 constant public ADD_REIMBURSEMENT_ROLE = keccak256("ADD_REIMBURSEMENT_ROLE");
bytes32 constant public ALLOWED_TOKENS_MANAGER_ROLE = keccak256("ALLOWED_TOKENS_MANAGER_ROLE");
bytes32 constant public CHANGE_PRICE_FEED_ROLE = keccak256("CHANGE_PRICE_FEED_ROLE");
bytes32 constant public MODIFY_RATE_EXPIRY_ROLE = keccak256("MODIFY_RATE_EXPIRY_ROLE");

Remediation

In order to keep readability while slightly optimizing gas usage, the code can be changed to include a comment describing the string hashes:

/* Hardcoded constants to save gas
bytes32 constant public ADD_EMPLOYEE_ROLE = keccak256("ADD_EMPLOYEE_ROLE");
bytes32 constant public TERMINATE_EMPLOYEE_ROLE = keccak256("TERMINATE_EMPLOYEE_ROLE");
bytes32 constant public SET_EMPLOYEE_SALARY_ROLE = keccak256("SET_EMPLOYEE_SALARY_ROLE");
bytes32 constant public ADD_BONUS_ROLE = keccak256("ADD_BONUS_ROLE");
bytes32 constant public ADD_REIMBURSEMENT_ROLE = keccak256("ADD_REIMBURSEMENT_ROLE");
bytes32 constant public ALLOWED_TOKENS_MANAGER_ROLE = keccak256("ALLOWED_TOKENS_MANAGER_ROLE");
bytes32 constant public CHANGE_PRICE_FEED_ROLE = keccak256("CHANGE_PRICE_FEED_ROLE");
bytes32 constant public MODIFY_RATE_EXPIRY_ROLE = keccak256("MODIFY_RATE_EXPIRY_ROLE");
*/
bytes32 constant public ADD_EMPLOYEE_ROLE = 0x9ecdc3c63716b45d0756eece5fe1614cae1889ec5a1ce62b3127c1f1f1615d6e;
bytes32 constant public TERMINATE_EMPLOYEE_ROLE = 0x69c67f914d12b6440e7ddf01961214818d9158fbcb19211e0ff42800fdea9242;
bytes32 constant public SET_EMPLOYEE_SALARY_ROLE = 0xea9ac65018da2421cf419ee2152371440c08267a193a33ccc1e39545d197e44d;
bytes32 constant public ADD_BONUS_ROLE = 0xceca7e2f5eb749a87aaf68f3f76d6b9251aa2f4600f13f93c5a4adf7a72df4ae;
bytes32 constant public ADD_REIMBURSEMENT_ROLE = 0x90698b9d54427f1e41636025017309bdb1b55320da960c8845bab0a504b01a16;
bytes32 constant public ALLOWED_TOKENS_MANAGER_ROLE = 0xaf745f37ed66a453b76a80330aadeb12ce3d4046a1bb2da44b80198c35acb8fa;
bytes32 constant public CHANGE_PRICE_FEED_ROLE = 0xc542dd99403fb00292dc8387e408d46f2964cb93af19a921ab5a1a399540ebe6;
bytes32 constant public MODIFY_RATE_EXPIRY_ROLE = 0x79fe989a8899060dfbdabb174ebb96616fa9f1d9dadd739f8d814cbab452404e;

This version will save some gas (~80 per constant usage) but it will force the code reviewer to validate the correctness of the computed values.

@facuspagnuolo facuspagnuolo requested a review from sohkai Jul 3, 2019

@facuspagnuolo facuspagnuolo self-assigned this Jul 3, 2019

@sohkai

sohkai approved these changes Jul 3, 2019

Copy link
Member

left a comment

👍

@coveralls

This comment has been minimized.

Copy link

commented Jul 3, 2019

Coverage Status

Coverage increased (+0.5%) to 98.232% when pulling 9fe897c on payroll/hardcode_role_constants into b989638 on master.

2 similar comments
@coveralls

This comment has been minimized.

Copy link

commented Jul 3, 2019

Coverage Status

Coverage increased (+0.5%) to 98.232% when pulling 9fe897c on payroll/hardcode_role_constants into b989638 on master.

@coveralls

This comment has been minimized.

Copy link

commented Jul 3, 2019

Coverage Status

Coverage increased (+0.5%) to 98.232% when pulling 9fe897c on payroll/hardcode_role_constants into b989638 on master.

@facuspagnuolo facuspagnuolo merged commit ebaed17 into master Jul 3, 2019

5 of 6 checks passed

License Compliance Analysis done, scanning for issues...
Details
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
coverage/coveralls Coverage increased (+0.5%) to 98.232%
Details
license/cla Contributor License Agreement is signed.
Details

@facuspagnuolo facuspagnuolo deleted the payroll/hardcode_role_constants branch Jul 3, 2019

@luisivan luisivan referenced this pull request Jul 5, 2019

Open

Payroll #6

14 of 20 tasks complete
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.