fix: returnAsset exploit

[xenide]

Motivation

• bug was due to

uint256 lCurrentShares = shares[aPair][aToken];
// this is to prevent underflow as we round up in the previous division operation
if (rShares > lCurrentShares) {
   rShares = lCurrentShares;
}

• code was introduced in test: asset manager corner cases #187

• mulDivUp behavior

  • does not round up if division is exact, only rounds up when there is remainder

• in our current production deployment of asset manager, all pairs have redeemed successfully (tokens managed are all 0 from the perspective of the pair), but individual and total shares are more than 0. Of course we have a special case where there was a loss (wasn't strictly increasing)

Potential Solutions

1. add a condition to check that lCurrentShares is greater than 0

if (rShares > lCurrentShares && lCurrentShares > 0) 

• this is not secure, as lCurrentShares can be 1, and rShares can be a really large number. i.e. an attacker can get several shares by letting the asset manager manage its assets, and then still attack using returnAsset

2. remove rShares > lCurrentShares check completely, relying on checked arithmetic.

• this is the recommended solution (implemented in this PR), as underflow will occur in the case of attempting to redeem 0 shares
• I've attempted to break this by fuzzing 100K times. So far none of them have triggered the subtraction underflow problem
  • the total number of shares and individual number of shares may not end up being 0, but at least the token0/1Managed will be zero.
    • this is only happening for very small amounts (e.g. amounts less than 10 wei)
• are there specific numbers to break it?

3. any other solutions you can think of?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.08%. Comparing base (5fe192e) to head (631803e).

❗ Current head 631803e differs from pull request most recent head 4ab4bc0. Consider uploading reports for the commit 4ab4bc0 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##              dev     #222       +/-   ##
===========================================
+ Coverage   68.21%   88.08%   +19.86%     
===========================================
  Files          12       12               
  Lines         623      621        -2     
===========================================
+ Hits          425      547      +122     
+ Misses        198       74      -124     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[OliverNChalk]

This change was originally introduced to ensure a full redemption was possible right? I think provided we have a way to recover if this maths breaks in full redemption mode then we can be reasonably comfortable. I.e. it may make sense to give the admin super user style power to make calls from the asset manager if needed?

You could imagine a function like this:

function rawCall(address target, uint256 value, bytes data) external onlyOwner

[xenide]

So previously when we did the PR, we were relying on an _getExchangeRate internal function, which was only accurate to 18 decimal places, which probably resulted in a few wei of difference.

Then during the audit, the auditors recommended us to perform the arithmetic directly (without using an intermediate exchange rate) which probably eliminated this problem. That said, with the testing I've done, I haven't been able to come up with a scenario in which full redemption will fail.

Yes I thought of the same regarding introducing rawCall too. If we put it behind a timelock I think it's acceptable to everyone. Will implement

[OliverNChalk]

So previously when we did the PR, we were relying on an _getExchangeRate internal function, which was only accurate to 18 decimal places, which probably resulted in a few wei of difference.

Then during the audit, the auditors recommended us to perform the arithmetic directly (without using an intermediate exchange rate) which probably eliminated this problem. That said, with the testing I've done, I haven't been able to come up with a scenario in which full redemption will fail.

Yes I thought of the same regarding introducing rawCall too. If we put it behind a timelock I think it's acceptable to everyone. Will implement

Assuming the owner is the timelock, it should be acceptable?