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: Remove allowed tokens #901

Merged
merged 2 commits into from Jul 5, 2019

Conversation

Projects
None yet
3 participants
@facuspagnuolo
Copy link
Contributor

commented Jul 3, 2019

Fixing finding 6.5 of the audit report

⚠️ Note that I had to make other changes to apply this one since we're hitting the max size limit of a contract. For instance, I reduce the getEmployeeByAddress to simply getEmployeeIdByAddress.

6.5 Allowed tokens list should be modifiable

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

Description

In the current implementation, there can only be a maximum of 20 allowed tokens that are used for payroll. There is no way to modify the allowedTokens list except for adding new tokens before the number of allowed tokens hits 20.

Examples

  1. If there will be more than 20 tokens (e.g. stablecoins) bounded to the real-world currencies.
  2. If the company switches multiple times from some set of coins to another.
  3. The company does not want to make a payroll in some tokens anymore.
  4. The address of a specific token is changed due to smart contract updates.

Remediation

Allow modifying allowedTokens list while maintaining security. Modifying the list is tricky and might introduce new attack vectors by employer, as the employee payment allocation is currently bounded to the order of allowed tokens list.

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

@facuspagnuolo facuspagnuolo self-assigned this Jul 3, 2019

@facuspagnuolo facuspagnuolo force-pushed the payroll/remove_allowed_tokens branch from e969e14 to d8b66d9 Jul 3, 2019

@coveralls

This comment has been minimized.

Copy link

commented Jul 4, 2019

Coverage Status

Coverage remained the same at 97.732% when pulling f9c0557 on payroll/remove_allowed_tokens into 4207ef0 on master.

@luisivan luisivan referenced this pull request Jul 5, 2019

Open

Payroll #6

15 of 20 tasks complete
@sohkai

sohkai approved these changes Jul 5, 2019

Copy link
Member

left a comment

❤️ Hadn't occurred to me that we could check the distribution in payout() 👍!

Have a couple of minor comments on naming / etc.

Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol Outdated
Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol Outdated
Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol Outdated
Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol Outdated
Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol Outdated

@facuspagnuolo facuspagnuolo merged commit 70e9264 into master Jul 5, 2019

5 of 6 checks passed

License Compliance FOSSA is analyzing this commit
Details
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
coverage/coveralls Coverage remained the same at 97.732%
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the payroll/remove_allowed_tokens branch Jul 5, 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.