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

Calculation imprecision when calculating the reaming cut #117

Open
code423n4 opened this issue Jun 16, 2021 · 1 comment
Open

Calculation imprecision when calculating the reaming cut #117

code423n4 opened this issue Jun 16, 2021 · 1 comment

Comments

@code423n4
Copy link
Contributor

Handle

pauliax

Vulnerability details

Impact

There is an issue kinda similar to the one that is described here: https://github.com/code-423n4/2021-06-realitycards#balance_scalerent-collection-rounding-mismatch

When returning the rent or paying the winnings some precision is lost due to calculation errors:

   uint256 _remainingCut =
            ((uint256(1000) - artistCut) - affiliateCut) - cardAffiliateCut;

    uint256 _rentCollectedAdjusted =
            (_rentCollected * _remainingCut) / (1000);

A small amount of wei is lost from rentCollectedPerCard. In practice, this should be an amount of dust that you do not have to worry about.

Recommended Mitigation Steps

I am not sure of an elegant solution to this 'problem'. So far the straightforward solution I see is to separately calculate amounts of artistCut, creatorCut, affiliateCut, and cardAffiliateCut and then subtract these amounts from a total rentCollectedPerCard to calculate the _remainingCut. That will increase the precision but will also increase the gas costs as extra calculations are needed. I am not sure if that's worth it.

@Splidge
Copy link
Collaborator

Splidge commented Jun 18, 2021

I 100% agree that it's probably not worth the extra computations to collect the dust that's left over. I'm not even sure it's worth trying to have a withdrawDust function because of the security it would require and the gas to call it could be more than you'll get back.
Anyway, this isn't actually leaving the dust on the market, it's just a value that tells the Treasury what it can allow the users to withdraw, the tokens remain on the Treasury.

It could also be that this cancels out to a certain degree the issues we face with the rounding going the other way (the one you link to).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants