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: Support employee's min acceptable exchange rates #904

Merged
merged 11 commits into from Jul 11, 2019

Conversation

Projects
None yet
3 participants
@facuspagnuolo
Copy link
Contributor

commented Jul 4, 2019

Fixing finding 6.2 of the audit report

6.2 Employee should be able limit exchange rate

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

Description

An employee has no control over the exchange rate feed and there is (almost) no way to ensure that the payroll will not be transferred using very unfair rate.

Example

Just like when you trade on the exchange, you usually submit a maximum price for which you are willing to buy the tokens, any employee should also be able to limit losses from untrusted exchange rate feeds.

Remediation

Add the ability for an employee to submit minimal exchange rates that are acceptable for the payroll. This can be done by passing an array with minimal rates to payday function. If an employee does not want to pass the limit, it's possible to just send an empty array or array of zeros.

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

@facuspagnuolo facuspagnuolo self-assigned this Jul 4, 2019

@facuspagnuolo facuspagnuolo force-pushed the payroll/add_min_acceptable_exchage_rates branch from 21e067e to b4895d3 Jul 4, 2019

@coveralls

This comment has been minimized.

Copy link

commented Jul 4, 2019

Coverage Status

Coverage remained the same at 97.732% when pulling c233c44 on payroll/add_min_acceptable_exchage_rates into efa101d on master.

@luisivan luisivan referenced this pull request Jul 5, 2019

Open

Payroll #6

15 of 20 tasks complete
@sohkai
Copy link
Member

left a comment

Ahh, I think we may have miscommunicated a bit here.

What do you think about just adding the ability to specify a min rate in the payday() function (and detecting if it's an empty array to signal there's no floor)?

I don't like the approach of including this at the token distribution level because it sets the min rate at the wrong time. In my mind, the purpose of this floor is to guarantee that you won't get screwed by an adverse change in the pricefeed when attempting to cash out. If you think the current exchange rate of a token is too high, you simply choose to not invoke payday() :).

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

@facuspagnuolo facuspagnuolo changed the base branch from payroll/floor_payday_paid_amount to master Jul 7, 2019

@facuspagnuolo facuspagnuolo force-pushed the payroll/add_min_acceptable_exchage_rates branch from b4895d3 to 408158a Jul 7, 2019

@facuspagnuolo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

@sohkai as discussed offline, I move forward with the idea of keeping a list of the tokens picked for payments per employee

sohkai added some commits Jul 9, 2019

@sohkai

sohkai approved these changes Jul 9, 2019

Copy link
Member

left a comment

🌟 I really like this approach for keeping the tokens per-employee rather than globally!

Show resolved Hide resolved future-apps/payroll/test/contracts/Payroll_gas_costs.test.js Outdated
Show resolved Hide resolved future-apps/payroll/test/contracts/Payroll_rates.test.js 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

@facuspagnuolo facuspagnuolo force-pushed the payroll/add_min_acceptable_exchage_rates branch from c8dee4d to f887e6d Jul 10, 2019

@facuspagnuolo facuspagnuolo merged commit 72736f9 into master Jul 11, 2019

5 of 6 checks passed

License Compliance 1 issues found
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

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