Skip to content
This repository has been archived by the owner on Mar 1, 2022. It is now read-only.

Investigate multiplication overflow issue #203

Closed
josojo opened this issue May 4, 2020 · 10 comments
Closed

Investigate multiplication overflow issue #203

josojo opened this issue May 4, 2020 · 10 comments
Assignees

Comments

@josojo
Copy link
Contributor

josojo commented May 4, 2020

Because we can run into multiplication overflows during the utility-calculation, we should no longer use them

@bh2smith
Copy link
Contributor

bh2smith commented May 4, 2020

This is cool, but aren't there other places where unlimited orders are placed?

@fleupold
Copy link
Contributor

fleupold commented May 4, 2020

See gnosis/dex-open-solver#64 for more details

@bh2smith
Copy link
Contributor

bh2smith commented May 5, 2020

Putting this on hold until we have a conclusion for our investigation of gnosis/dex-open-solver#64

@fleupold
Copy link
Contributor

fleupold commented May 6, 2020

I think at least for the OWL liquidity we will need this anyways (maybe the max-amount can be configurable)

@fedgiac
Copy link
Contributor

fedgiac commented May 6, 2020

We could easily lower this amount by a very large margin (10⁹) without compromising much on our selling point of not requiring any maintenance. This margin should be enough to avoid the issue we are having with unlimited orders, at least for the main tokens that are traded in the exchange.

More concretely, decreasing the magic number for unlimited orders by 10⁹ would allow a user to swap back and forth at every batch for more than one year an amount of one million of a 18-decimal token without renewing the orders. To verify that with the node console:

const maxAmount = 2n**128n;
const batchesInAYear = 365n*24n*60n/5n;
const lowerBoundSizeOfUnit = 10n**18n;
const amountTradedEachBatch = 10n**6n;
maxAmount / batchesInAYear / lowerBoundSizeOfUnit / amountTradedEachBatch > 10**9

An order is just the two amounts of tokens to be swapped against each other.

The situation is worse for tokens with more decimals and tokens that are very cheap because a user would have to swap more token units to move the same amount of "value" compared to a more standard token.

@fleupold
Copy link
Contributor

fleupold commented May 6, 2020

Looking at gnosis/dex-open-solver#64 it looks like a significant factor for the issue we were seeing was that we were trading with ourselves (and since the solver does not "exploit" trades with one-selves by maxing out the potential trading volume until all balance is eaten up by fees) we had a large disregarded utility.

I'm no longer sure we actually need to do anything here. On the other hand making the amount configurable seems simple enough and could be useful to have in any case.

@bh2smith
Copy link
Contributor

bh2smith commented May 7, 2020

I will get on this today! Lies, I will nudge @fedgiac and see if he is interested.

@fedgiac
Copy link
Contributor

fedgiac commented May 11, 2020

Further discussion is needed before getting rid of unlimited orders, since it's not clear how frequently the issue occurs and whether this change would ameliorate the issue. For now we keep unlimited orders until the issue presents itself again.

@bh2smith
Copy link
Contributor

bh2smith commented Jun 3, 2020

A good suggestion of @fedgiac, would be that perhaps we could get a good idea of where to go if we can force this scenario to happen on the testnet.

@bh2smith bh2smith removed the on hold label Jun 3, 2020
@fedgiac fedgiac changed the title do no longer place unlimited orders Investigate multiplication overflow issue Jun 8, 2020
@bh2smith
Copy link
Contributor

@fedgiac How is this coming along? Do we have an open PR with some code related to the investigation?

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

Successfully merging a pull request may close this issue.

4 participants