fix to dust issue on unforced withdrawals under special case

[dxganta]

Fix to Audit issue #13 according to recommendation

[gabrielpoca]

Was this test failing without your changes?

[dxganta]

This test because of the subtraction of 1 done in line 2977 to take the dust into account. Similarly many other tests also had a +1 or -1 in the end to account for the dust. This fix, fixes that issue so now we dont have to account for dust in the tests thus also making the tests more readable and simpler. I have made the changes to the tests and they are passing now.

[gabrielpoca]

This change violates one of our invariants: deposits or withdrawals should not affect the price per share.

[dxganta]

Just curious to why was there a test for "pps can be manipulated"?

[gabrielpoca]

It's a test from the 2nd audit we did. The test proved (at the time) that it could be manipulated. We just kept it as it was

[gabrielpoca]

@dxganta check my comment above regarding the invariant

[dxganta]

This change violates one of our invariants: deposits or withdrawals should not affect the price per share.

I assume you meant this test.

It was failing due to this line where at the end 1 was subtracted from the oldBalance variable to account for the difference in test. Since the current commit, fixes this so the we can assertunderlying.balance == oldBalanceinstead of doing underlying.balance == oldBalance-1.

I thus removed the sub(1) from the test and the test is passing now. Its not violating the invariant.

[gabrielpoca]

@dxganta I'm not following. There are many more tests about that. That's on purpose, and for the reason I said: no user action should manipulate the price per share. Your change breaks that rule. @fyang1024 since you opened the issue, can you jump in here?

[dxganta]

@dxganta I'm not following. There are many more tests about that. That's on purpose, and for the reason I said: no user action should manipulate the price per share. Your change breaks that rule. @fyang1024 since you opened the issue, can you jump in here?

No I dont think it does. All the tests I changed are just removing that '1' wei dust from the assertion which was there to account for the dust which is currently solved now with this fix. For example, in this test itself,

a deposit is being made in line 139 and then it is withdrawn in line 153, thus the balance before deposit must equal the balance after withdraw and this is being asserted in line 155. But currently in the test instead of testing oldBalance==newBalance we are testing oldBalance==newBalance-1 , that 1 being the dust. I just removed that -1 and now we are testing oldBalance==newBalance which is what was meant to test by that particular test anyway if I read the code correctly.

[gabrielpoca]

I'll wait for @fyang1024 to answer before I go into details. But that -1 in the tests is supposed to be there. It's not dust. It's the guarantee that the price per share is not affected. The numbers in the tests are not entirely divisible by each other, and therefore it's expected to have "dust".

[fyang1024]

Hi @gabrielpoca , as Certora has proved, the current implementation cannot preserve exact share price when user makes withdrawals because of the possible rounding of uint division in the _computeAmount function. I don't think it's achievable, hence I specified a minor difference allowance for share price after withrawal in the Certora spec .

The changed withdraw function won't be able to preserve the share price because of the possible rounding of uint division in the withdraw function as well, but it's unavoidable anyway. The changed function will solve the dust issue though.

[gabrielpoca]

@fyang1024 yeah, I know that you lose precision every time you change from the amount to shares or shares to the amount. But both implementations are different because on the previous one, we always rounded down, but on this one, you are rounding up.

[fyang1024]

@gabrielpoca I get your point.

let's use an example.

1. Alice initially deposits 1000 Token, the her shares is 1000 * 10^18.
2. Then the deposit generates some yield, now the vault has 1002 Token.
3. Now Alice wants to withdraw the 1000 Token.

• The previous implementation firstly calculates the current shares of the 1000 token based on the latest share price, that's 1000 * (1000 * 10 ^ 18) / 1002 = 9.980039920159679 * 10^20, and then calculates the amount using the current share price, that's 9.980039920159679 * 10^20 * 1002 / (1000 * 10^18) = 999.9999999999998 Token which will be round down to 999. The actual share price Alice gets is below the current share price.

• The updated implementation allows Alice to withdraw the initial 1000 Token, which will make her share price slightly above the current share price.

Neither implementations maintain the share price invariant, but I don't see why previous round down makes the Vault better.
I don't think rounding down or up matters here (max 1 wei difference), unless an exploit exists for the round up solution.

[gabrielpoca]

My issue with the rounding up is that you can only round up for some. For instance, if the vault loses funds, and everyone is in a rush to withdraw from the vault, only some people will be able to round up, and the others will have to round down since there will be no funds left. Worst case scenario, the last person to withdraw gets burned, right?

A second concern is really about the price per share. While initially, we have 1 share = 1000000000000000000 underlying, that relationship will change, and if the vault really really well, the rounding up may no longer be ~0.000000000000000001 underlying, and instead turn into a bigger amount. What do you think?

[fyang1024]

The bottom line is that the user can withdraw the full amount only if the vault is making money and the user cannot withdraw more than her/his initial deposit.

In the example above, the previous implementation burned the only user slightly without benefiting anyone else. I don't think 1 wei difference will make the last person to withdraw gets burned too much in practice.

When the share price rises a lot, the two implementations will most likely end up with the same. I tried to assume the step 2 of the example has 1299 Token or 1999 Token rather than 1002 Token, and the calculations of both end up with the same which both allow the user to withdraw all the initial deposit.

[gabrielpoca]

You are correct that you can only withdraw total amounts if the vault didn't lose money. I mean, it's only half true since there's also the local loss, where if the vault makes a lot of money and then loses only a bit after a claim, you can't withdraw your total amount. But it still doesn't change the fact that with each deposit you are slightly decreasing the price per share, right?

[fyang1024]

If the vault is losing money, and the user did forceWithdraw, the 2 implementations have the same calculation, as the updated one has to recalculate the amount based on the shares that get haircut. Or the user cannot withdraw any.

If the vault is making a lot of money, the 2 implementations should end up with the same, even though the calculations are slightly different.

The two implementations have slightly different results only when the share price rises slightly (e.g, the user deposited yesterday and withdrew today)

[gabrielpoca]

But withdrawals in this solution will decrease the price per share, while in the previous solution, they would increase it even if it's only so slightly for each withdrawal. For instance, if 3 million withdrawals are happening while the vault is also losing money (not enough to get negative), the withdrawals will help the price per share go down. I can try and run some numbers on this because I'm sure I could be missing some obvious property that prevents this.