VaultTracker miscalculates compounding interest

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/main/VaultTracker/VaultTracker.sol#L65 https://github.com/code-423n4/2022-07-swivel/blob/main/VaultTracker/VaultTracker.sol#L100 https://github.com/code-423n4/2022-07-swivel/blob/main/VaultTracker/VaultTracker.sol#L130 https://github.com/code-423n4/2022-07-swivel/blob/main/VaultTracker/VaultTracker.sol#L172 https://github.com/code-423n4/2022-07-swivel/blob/main/VaultTracker/VaultTracker.sol#L191 https://github.com/code-423n4/2022-07-swivel/blob/main/VaultTracker/VaultTracker.sol#L228

Vulnerability details

Impact

VaultTracker neglect previously accrued interest while attempting to calculate new interest. This causes nToken holders to receive less yield than they should.

All functions within VaultTracker that calculate interest are affected, including addNotional, removeNotional, redeemInterest, transferNotionalFrom and transferNotionalFee.

Proof of Concept

Consider the case where some user N tries to initiate a vault at 3 specific moments where cToken exchange rate is 5/10/20 respectively. The corresponding market stays active and has not reached maturity. Additionally, N selects his premium volume to make principalFilled match the cToken exchange rate during each call to initiateVaultFillingZcTokenInitiate. We recognize those exchange rates are most likely unrealistic, but we chose those for ease of demonstrating the bug. We also assume fees to be 0 for simplicity.

For the first call to Swivel.deposit

• a = 5
• exchangeRate = 5

Assuming no additional fees while minting cToken, N will receive `cToken for his 5 underlying tokens.

For the matching call to VaultTracker.addNotional

• a = 5
• vlt.notional = 0
• exchangeRate = 5

Since this is the first time adding nToken to the vault, there is no need to consider any accumulated interests, and we can assign a directly to vlt.notional.

The result will be

• vlt.notional = 5
• vlt.redeemable = 0
• cToken held by Swivel = 1

For the second call to `Swivel.deposit, we have

• a = 10
• exchangeRate = 10

The matching call to VaultTracker.addNotional has

• a = 10
• vlt.notional = 5
• vlt.redeemable = 0
• exchangeRate = 10
• vlt.exchangeRate = 5

The yield is derived from ((exchangeRate * 1e26) / vlt.exchangeRate) - 1e26 = ((10 * 1e26) / 5) - 1e26 = 1e26 Applying this to vlt.notional, we get interest = 1e26 * 5 / 1e26 = 5

This results in

• vlt.notional = 5+10 = 15
• vlt.redeemable = 0+5 = 5
• cToken held by Swivel = 1+1 = 2

Now comes the last call to Swivel.deposit, where

• a = 20
• exchangeRate = 20

VaultTracker.addNotional has

• a = 20
• vlt.notional = 15
• vlt.redeemable = 5
• exchangeRate = 20
• vlt.exchangeRate = 10

yield = ((20 * 1e26) / 10) - 1e26 = 1e26 interest = 1e26 * 15 / 1e26 = 15

So we finally end up with

• vlt.notional = 15+20 = 35
• vlt.redeemable = 5+15 = 20
• cToken held by Swivel = 2+1 = 3

Swivel{
  ...
  function deposit(uint8 p, address u, address c, uint256 a) internal returns (bool) {
    ...
    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
      return ICompound(c).mint(a) == 0;
    }
    ...
  }
  ...
}

VaultTracker{
  ...
  function addNotional(address o, uint256 a) external authorized(admin) returns (bool) {
    uint256 exchangeRate = Compounding.exchangeRate(protocol, cTokenAddr);

    Vault memory vlt = vaults[o];

    if (vlt.notional > 0) {
      uint256 yield;

      // if market has matured, calculate marginal interest between the maturity rate and previous position exchange rate
      // otherwise, calculate marginal exchange rate between current and previous exchange rate.
      if (maturityRate > 0) { // Calculate marginal interest
        yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26;
      } else {
        yield = ((exchangeRate * 1e26) / vlt.exchangeRate) - 1e26;
      }

      uint256 interest = (yield * vlt.notional) / 1e26;
      // add interest and amount to position, reset cToken exchange rate
      vlt.redeemable += interest;
      vlt.notional += a;
    } else {
      vlt.notional = a;
    }
    vlt.exchangeRate = exchangeRate;
    vaults[o] = vlt;

    return true;
  }
    ...
}

Now take a step back and think about the actual value of 3 cToken when exchangeRate = 20, it should be pretty obvious that the value tracked by VaultTracker = 35 + 20 = 55 is lesser than actual value of cToken held by Swivel = 20*3 = 60.

This is due to VaultTracker neglecting that previously accrued interest should also be considered while calculating new interest.

Tools Used

Manual Review

Recommended Mitigation Steps

For all interest calculations, use vlt.notional + vlt.redeemable instead of just vlt.notional as yield base

[JTraversa]

I believe that this would be valid if the redeemable was not redeemable by the user at any point in time.

While interest accrues, it accrues to the redeemable balance which is withdrawn at any time.

That said, in most cases, the math required to store and do the additional calculation marginal interest on the redeemable balance is largely a UX consideration? Should users be required to redeem their redeemable to earn interest or should it be compounded naturally though bloat tx costs?

Should we force additional costs on a per transaction basis (that likely costs more than the interest itself for the vast majority of users), or should we assume that once significant enough redeemable is accrued to earn reasonable further interest, it will be redeemed by the user.

(e.g. with example being 400% interest, and assuming an (optimistic) 5% APY is possible, the example would take ~28 years to replicate, and gas costs for transactions in that 28 years would be significant)

[JTraversa]

Thought about this one a bit more and it might slip in as acknowledged and disagreed with severity as its more value leakage than anything else.

Its a design decision, but could still be considered a detriment so perhaps not worth disputing all together?

[bghughes]

I agree with the warden here and regarding:

I believe that this would be valid if the redeemable was not redeemable by the user at any point in time.

While interest accrues, it accrues to the redeemable balance which is withdrawn at any time.

That said, in most cases, the math required to store and do the additional calculation marginal interest on the redeemable balance is largely a UX consideration? Should users be required to redeem their redeemable to earn interest or should it be compounded naturally though bloat tx costs?

Should we force additional costs on a per transaction basis (that likely costs more than the interest itself for the vast majority of users), or should we assume that once significant enough redeemable is accrued to earn reasonable further interest, it will be redeemed by the user.

(e.g. with example being 400% interest, and assuming an (optimistic) 5% APY is possible, the example would take ~28 years to replicate, and gas costs for transactions in that 28 years would be significant)

you should aspire to use the correct math so your users get the best rate (inclusive of compounding), respectfully. IMO what the warden suggests is what the protocol should do given the entire reason many lenders deposit into a yield token is to stack compounding interest. This could lead to misleading users and compounding loss of value.

[robrobbins]

the above is nonsense

this is a design feature. we could just as easily dismiss this all together by stating it is now documented that .notional is non compounding. i don't think we should do that however as i agree with @JTraversa here that some acknowledgement should happen for bringing this up. but it is not a risk in any situation

[robrobbins]

addressed: https://github.com/Swivel-Finance/gost/pull/427

[0xean]

going to reduce the severity on this to medium as it has some pretty large external factors to end up in a scenario where it leaks any real value.