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

Audit review 6.5: Payroll is missing permissions to create payments on Finance #151

Merged
merged 2 commits into from Sep 3, 2019

Conversation

@facuspagnuolo
Copy link
Member

commented Sep 2, 2019

⚠️ Caveat

Problem

Employees will not be able to get their salary as Payroll does not have permissions on Finance to create payments. This permission is needed in order to pay out employee salaries when an employee calls Payroll.payday().

Solution

Grant CREATE_PAYMENT permission on Finance for Payroll. Note even though Payroll allows employees to call evmScripts, interaction with Finance via forward() is blacklisted.

Audit issue

https://github.com/aragonone/aragon-daotemplates-audit-report-2019-08#65-payroll-is-missing-permissions-to-create-payments-on-finance

@facuspagnuolo facuspagnuolo requested review from ajsantander and sohkai Sep 2, 2019

@facuspagnuolo facuspagnuolo self-assigned this Sep 2, 2019

@facuspagnuolo facuspagnuolo changed the title Templates: Grant create payments permission to Payroll Audit review 6.5: Payroll is missing permissions to create payments on Finance Sep 2, 2019

@facuspagnuolo facuspagnuolo force-pushed the grant_create_payments_permission_to_payroll branch from 295f14a to f590520 Sep 2, 2019

@ajsantander ajsantander changed the base branch from master to audit Sep 2, 2019

@facuspagnuolo facuspagnuolo force-pushed the grant_create_payments_permission_to_payroll branch 2 times, most recently from a0fba52 to b4390c6 Sep 2, 2019

@CLAassistant

This comment has been minimized.

Copy link

commented Sep 2, 2019

CLA assistant check
All committers have signed the CLA.

@facuspagnuolo facuspagnuolo force-pushed the grant_create_payments_permission_to_payroll branch from b4390c6 to fcd745a Sep 2, 2019

@facuspagnuolo facuspagnuolo force-pushed the grant_create_payments_permission_to_payroll branch from fcd745a to bd18176 Sep 2, 2019

@ajsantander
Copy link
Contributor

left a comment

Looks good! I think that it makes sense to spend a little more gas instead of providing a partially set up Payroll app.

@facuspagnuolo we should update the "Additional permissions if the Payroll app is installed" table in each of the READMEs.

@sohkai
sohkai approved these changes Sep 3, 2019
Copy link
Member

left a comment

🙌

@facuspagnuolo facuspagnuolo merged commit a2c20eb into audit Sep 3, 2019

4 checks passed

WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@facuspagnuolo facuspagnuolo deleted the grant_create_payments_permission_to_payroll branch Sep 3, 2019

facuspagnuolo added a commit that referenced this pull request Sep 3, 2019
Audit review 6.5: Payroll is missing permissions to create payments o…
…n Finance (#151)

* templates: grant create payments permission to payroll

* chore: update readmes permissions
facuspagnuolo added a commit that referenced this pull request Sep 4, 2019
Audit review 6.5: Payroll is missing permissions to create payments o…
…n Finance (#151)

* templates: grant create payments permission to payroll

* chore: update readmes permissions
facuspagnuolo added a commit that referenced this pull request Sep 4, 2019
Audit review 6.5: Payroll is missing permissions to create payments o…
…n Finance (#151)

* templates: grant create payments permission to payroll

* chore: update readmes permissions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.