Audit #22
Audit #22
Conversation
| // [audit]: I'm still not sure how to best multiply and divide by n^n | ||
| // In curve, they've changed this 6 months ago to dividing by n^n at the end: | ||
| // https://github.com/curvefi/stableswap-ng/commit/7ac164bb7d9cf800d50adf549f10539684db69b8 | ||
| // (supposedly after some large audit). | ||
| // I think it makes sense, as for n <= 8, the value of n^n is not too big, so we don't save much | ||
| // by dividing product by it when iterating (as it is currently), but we might | ||
| // run into some precision issues instead, when amount*n ~ d_prod. |
There was a problem hiding this comment.
argument in favor of dividing by n^n is that (simplest example for 2 tokens) when you have A, B and want to compute AB/n^n = AB/4, it's possible that A is divisible by 4 and B is not divisible by 2. Then you don't lose any precision when dividing AB by 4, but you lose when you divide B by 2.
Haven't found argument in favor of dividing each by n except avoiding overflow, but I think it shouldn't happen. D / x should be relatively small (at least at the first iteration)
| // [audit] then we first update rate successfully, then rate changed | ||
| // will be set to true and no further rate.update_rate calls will be done in this loop. | ||
| // Not sure if this is what we meant - maybe bitwise `or` operator (`|`) should be used? |
| return Ok(d); | ||
| } | ||
| d = d_next; | ||
| } | ||
| // [audit] Shouldn't we return precision error here? |
There was a problem hiding this comment.
That's a good point. It was implemented this way so it would always be possible for LPs to withdraw, like in Ref implementation. Later, withdraw_liquidity_by_share(...) was added which allows withdrawing without calculating invariant so it makes sense to return an error here.
| @@ -287,6 +287,9 @@ pub mod stable_pool { | |||
| let current_time = self.env().block_timestamp(); | |||
| let mut rate_changed = false; | |||
| for rate in self.pool.token_rates.iter_mut() { | |||
| // [audit] then we first update rate successfully, then rate changed | |||
| // will be set to true and no further rate.update_rate calls will be done in this loop. | |||
| // Not sure if this is what we meant - maybe bitwise `or` operator (`|`) should be used? | |||
There was a problem hiding this comment.
nice, very cheeky one, I would suggest not using bit operators to fix it, but some simple if else
| } | ||
| } else if d.checked_sub(d_next).ok_or(MathError::SubUnderflow(3))? <= 1.into() { | ||
| // [nit] Maybe like this? | ||
| if d_next.abs_diff(d) <= 1.into() { |
There was a problem hiding this comment.
I was looking for this the first time I reviewed the code and then forget to comment about it <3
|
Some takeaways from discussion with Jan Set fee ability can make ppl scaredCurrently there's no limit on the
Splitting ownership / roles / 2 step ownership transferOwnership transfer is pretty primitive in our case - there's owner who can set owner arbitrarily. Owner is also pretty powerful, and there are no other roles - they can manipulate Cross calls to rate providerCurrently rate is cached per 24h. The first person which uses our contract after the caching period has to pay additional gas for a cross contract call (which updates the cached value).
How to solve... I don't think it's possible to somehow bring justice here, the only potential way is to have own 'updaters' bots which will make the rate in cache updated. Admin fees are stored as lp tokensCurrently fees which go to admin are stored as lp tokens instead of the regular tokens. This means admin can choose their tokens of preference when exchanging the lp tokens for the regular tokens. This is a difference between us & ref, and curve. In curve admin can only withdraw normal tokens which are collected per swap. Likely this is not an issue - given that fees are small, admin won't be able to harm the system by selecting a token in which they want to obtain the fee. |
I agree that fee should be capped. In our first AMM implementation we use u8 to represent the percentiles of the fee so it's capped at 256/1000.
2-step ownership is a pain to manage but I don't disagree it's useful. I definitely agree about splitting owner into separate roles for fee management etc.
This is unavoidable, as you note. We should start w/ measuring the costs between swaps that update the rates and those that do not – I don't think the difference will be significant.
Technically the approach is different but I don't think it's different on the "token" level. If you withdrew in underlying tokens you can always swap to the one you want (ending up with just 1 token you desire) - I think you'd get the same rate as you get w/ the current approach b/c withdrawing one-sided tokens calculates the same exchange rate. |
No description provided.