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: Add payroll remainders as accrued salary #903

Merged

Conversation

Projects
None yet
2 participants
@facuspagnuolo
Copy link
Contributor

commented Jul 4, 2019

Fixing finding 6.3 of the audit report

6.3 Employee may lose up to a second of the salary

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

Description

An employee can lose up to a second of their salary on calling payday function. It happens if the employee requests a specific amount of money that cannot be formed by multiplying salary-per-second base-unit denominationTokenSalary and any amount of seconds. On that case, rounding is done in favour of an employer.

code/payroll/future-apps/payroll/contracts/Payroll.sol:L751-L759

uint256 timeDiff = _paidAmount.div(employee.denominationTokenSalary);

// We check if the division was perfect, and if not, take its ceiling to avoid giving away
// tiny amounts of salary.
// Employees may lose up to a second's worth of payroll if they use the "request partial
// amount" feature or reach salary amounts that get capped by uint max.
if (timeDiff.mul(employee.denominationTokenSalary) != _paidAmount) {
    timeDiff = timeDiff.add(1);
}

This issue has also been discussed by the project team.

Remediation

While losing up to a second might not sound like a big issue, it can be easily fixed by adding this rounding error to the accrued salary of the employee instead of just keeping it to the employer. Adding this to the accrued salary, might have side-effects e.g. that a terminated employee cannot immediately be removed from storage after cashing out, because payday accounted the lost fraction of a second to accrued salary.

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

@facuspagnuolo facuspagnuolo self-assigned this Jul 4, 2019

@facuspagnuolo facuspagnuolo force-pushed the payroll/floor_payday_paid_amount branch from fd8d67f to 8fd4b94 Jul 4, 2019

@aragon aragon deleted a comment from coveralls Jul 4, 2019

@luisivan luisivan referenced this pull request Jul 5, 2019

Open

Payroll #6

14 of 20 tasks complete
@sohkai
Copy link
Member

left a comment

A couple of suggestions on making this a bit nicer.

See #906 for changes that would invalid these comments.

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

Thanks @sohkai for #906, it looks much clearer now 😄

@facuspagnuolo facuspagnuolo force-pushed the payroll/use_ppf_rate_precision branch from 65980f3 to dd029f9 Jul 5, 2019

@facuspagnuolo facuspagnuolo force-pushed the payroll/floor_payday_paid_amount branch from 5bd0156 to 42ef977 Jul 5, 2019

@facuspagnuolo facuspagnuolo force-pushed the payroll/use_ppf_rate_precision branch from dd029f9 to d28394c Jul 5, 2019

@facuspagnuolo facuspagnuolo force-pushed the payroll/floor_payday_paid_amount branch from 42ef977 to 4336506 Jul 5, 2019

@sohkai

sohkai approved these changes Jul 5, 2019

@facuspagnuolo facuspagnuolo merged commit bc799c4 into payroll/use_ppf_rate_precision Jul 7, 2019

4 of 6 checks passed

coverage/coveralls Coverage decreased (-0.7%) to 97.015%
Details
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
license/cla Contributor License Agreement is signed.
Details

facuspagnuolo added a commit that referenced this pull request Jul 7, 2019

@facuspagnuolo facuspagnuolo deleted the payroll/floor_payday_paid_amount branch Jul 7, 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.