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: Use PPF rate precision #902

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.6 of the audit report

6.6 Decimal calculations ONE, can differ between Price feed and Payroll

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

Description

The decimal calculation is using a hardcoded ONE = 10 ** 18; in both PPF (uint256) and Payroll (uint128) contracts.

  • In PPF.sol:

code/ppf-contracts/packages/ppf-contracts/contracts/PPF.sol:L11

uint256 constant public ONE = 10 ** 18; // 10^18 is considered 1 in the price feed to allow for decimal calculations
  • In Payroll.sol:

code/payroll/future-apps/payroll/contracts/Payroll.sol:L32

uint128 internal constant ONE = 10 ** 18; // 10^18 is considered 1 in the price feed to allow for decimal calculations

This can result in an issue in some calculations as the types are different (uint128 v.s. uint 256) but mainly for future/thirdparty IFEED interfaces that specify different decimal points for ONE.

Examples

  • In PPF.sol:

code/ppf-contracts/packages/ppf-contracts/contracts/PPF.sol:L113-L116

function get(address base, address quote) public view returns (uint128, uint64) {
    if (base == quote) {
        return (uint128(ONE), getTimestamp64());
    }
  • In Payroll.sol:

code/payroll/future-apps/payroll/contracts/Payroll.sol:L627-L630

// Convert amount (in denomination tokens) to payout token and apply allocation
uint256 tokenAmount = _totalAmount.mul(exchangeRate).mul(tokenAllocation);
// Divide by 100 for the allocation percentage and by ONE for the exchange rate precision
tokenAmount = tokenAmount / (100 * ONE);

Remediation

In Payroll.sol where ONE is used, read the value from PPF contract, or simply require them to be the same.

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

@facuspagnuolo facuspagnuolo self-assigned this Jul 3, 2019

@facuspagnuolo facuspagnuolo force-pushed the payroll/use_ppf_rate_precision branch from 911f62e to 65980f3 Jul 3, 2019

@facuspagnuolo facuspagnuolo changed the base branch from master to payroll/remove_allowed_tokens 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 d28394c on payroll/use_ppf_rate_precision into 70e9264 on master.

@luisivan luisivan referenced this pull request Jul 5, 2019

Open

Payroll #6

14 of 20 tasks complete
@sohkai

sohkai approved these changes Jul 5, 2019

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

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

@facuspagnuolo facuspagnuolo changed the base branch from payroll/remove_allowed_tokens to next Jul 5, 2019

@facuspagnuolo facuspagnuolo changed the base branch from next to master Jul 5, 2019

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

@facuspagnuolo facuspagnuolo merged commit 474409c into master Jul 5, 2019

6 checks passed

License Compliance Analysis timed out. Check FOSSA for updates.
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
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.