fix: pool domination attack

[kosecki123]

This PR fixes the possible attack scenario such that:

• start pool with 2 assets, each having 100e18 tokens
• redeem 1 assets with amount 100e18-1

Pool now is locked, because of tiny current/total is 1, yielding 0 fee, which results in assert failing.

Context:
Protocol has the assert (fee > 0) to stop the situation where privileged user, miner/validator, can execute very small deposits/redemption (by splitting the amount into small chunks) and thus being able to infinitely modify the pool with 0 fees paid.

Proposed solution:
Since we cannot calculate fee, given the precision it yields 0. We resort to applying fixed fee which is equal to maximum fee for the redemption function, 30% currently.

Using 30% will not break the invariant such that multiple many small "dust" deposits will be cheaper than 1 deposit.

[kosecki123]

@PawelTroka need help fixes the failing test.

[kosecki123]

@0xmichalis please take a look, very interested in your opinion.

[PawelTroka]

@kosecki123 ok, but this won't work for e.g. amounts lower than 10, because 10% then yields 0 fee, right?
is this ok?

[kosecki123]

@kosecki123 ok, but this won't work for e.g. amounts lower than 10, because 10% then yields 0 fee, right? is this ok?

I think that's ok, this means that we can be left with tiny amount of credits which, unless we deposit more, we won't be able to redeem.

[PawelTroka]

so, is this ok if it rounds down to zero?

[kosecki123]

In such case, because of

require(totalFee > 0, "Fee must be greater than 0");

protocol will fail

This means we cannot support redemption of amount <= 10 from the highly dominated pool.

[PawelTroka]

Yes

[0xmichalis]

@kosecki123 when you say amount <= 10 that is in wei, right?

[0xmichalis]

@kosecki123

Pool now is locked, because of tiny current/total is 1, yielding 0 fee, which results in assert failing.

Just to understand this better, redeeming the asset that has 1 wei left in the pool is locked, right? The other asset still has 100e18 in the pool and that should be redeemable without an issue. Does that sound right?

[kosecki123]

@kosecki123

Pool now is locked, because of tiny current/total is 1, yielding 0 fee, which results in assert failing.

Just to understand this better, redeeming the asset that has 1 wei left in the pool is locked, right? The other asset still has 100e18 in the pool and that should be redeemable without an issue. Does that sound right?

No, redeeming any asset from the pool will fail, hence this is considered the attack.

[0xmichalis]

No, redeeming any asset from the pool will fail, hence this is considered the attack.

It'd be great to have a unit test that uses two mock tokens instead of one to showcase this. The test should fail in main but would presumably pass with your fix. I couldn't see how the allocation of other assets affected the fee calculation for an asset with the review I have done so far but need to have another look.

I see now. This is based on how the ratios are calculated in getRatiosRedemption where the total pool supply is used. Also the case seems to be covered by the unit test you are adding.

[PawelTroka]

lgtm