Swivel v3 contest - GimelSec's results

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

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/main/VaultTracker/VaultTracker.sol#L7 https://github.com/code-423n4/2022-07-swivel/blob/main/Marketplace/MarketPlace.sol#L8 https://github.com/code-423n4/2022-07-swivel/blob/main/Creator/Creator.sol#L41

Vulnerability details

Impact

Functions which call VaultTracker admin functions (e.g. addNotional) from MarketPlace will always revert since the admin is Creator.

Proof of Concept

VaultTracker.sol has an authorized(admin) modifier which only allows admin to call these functions. And the Creator will create VaultTracker, so the immutable admin of VaultTracker will always be Creator. But some functions in MarketPlace will also call these admin-only functions of VaultTracker, leading to these functions will always revert.

These lines below will call admin functions of VaultTracker in Marketplace/MarketPlace.sol:

120:    if (!IVaultTracker(market.vaultTracker).addNotional(t, a)) { revert Exception(25, 0, 0, address(0), address(0)); }
251:    if (!IVaultTracker(market.vaultTracker).addNotional(n, a)) { revert Exception(25, 0, 0, address(0), address(0)); }

136:    if (!IVaultTracker(market.vaultTracker).removeNotional(t, a)) { revert Exception(26, 0, 0, address(0), address(0)); }
269:    if (!IVaultTracker(market.vaultTracker).removeNotional(n, a)) { revert Exception(26, 0, 0, address(0), address(0)); }

203:    uint256 interest = IVaultTracker(markets[p][u][m].vaultTracker).redeemInterest(t);

102:    IVaultTracker(market.vaultTracker).matureVault(exchangeRate);

302:    if (!IVaultTracker(markets[p][u][m].vaultTracker).transferNotionalFrom(f, t, a)) { revert Exception(27, 0, 0, address(0), address(0)); }
316:    if (!IVaultTracker(markets[p][u][m].vaultTracker).transferNotionalFrom(msg.sender, t, a)) { revert Exception(27, 0, 0, address(0), address(0)); }

329:    IVaultTracker(markets[p][u][m].vaultTracker).transferNotionalFee(f, a);

Tools Used

Manual Review

Recommended Mitigation Steps

Set admin when Creator create VaultTracker, and add admin argument in VaultTracker constructor:

constructor(..., address _admin, ...){
    admin = _admin;
    ...
}

Summary

We list 1 low-critical finding and 2 non-critical findings:

• (Low) ZcToken will get wrong values due to the wrong maturityRate
• (Non) Swivel.setFee should check length <= feeNominator array length
• (Non) Using ecrecover is against best practice

(Low) ZcToken will get wrong values due to the wrong maturityRate

Impact

ZcToken will get wrong values if users don't call matureMarket after maturity.

Proof of Concept

https://github.com/code-423n4/2022-07-swivel/blob/main/Tokens/ZcToken.sol

In ZcToken.convertToUnderlying, ZcToken.previewRedeem and ZcToken.maxWithdraw, these functions will be reverted after maturity due to division by 0 if market is not matured.

In ZcToken.convertToPrincipal and ZcToken.previewWithdraw, these functions return incorrect value if they are called after maturity but the market is not matured. This will cause ZcToken.withdraw to pass incorrect value to authRedeem, and potentially wastes user gas.

Recommended Mitigation Steps

Check the market should be matured.

(Non) Swivel.setFee should check length <= feeNominator array length

Impact

It will be reverted if length > feeNominator array length.

Proof of Concept

https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L495

In setFee, the length of array i and d should <= 4 which is the feeNominator array length.

Recommended Mitigation Steps

Check i and d length <= feeNominator array length.

(Non) Using ecrecover is against best practice

Impact

Using ecrecover is against best practice. Preferably use ECDSA.recover instead. EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature unique. However it should be impossible to be a threat by now.

Proof of Concept

https://github.com/code-423n4/2022-07-swivel/blob/main/Tokens/Erc20.sol#L141

Recommended Mitigation Steps

Take these implementation into consideration

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4a9cc8b4918ef3736229a5cc5a310bdc17bf759f/contracts/utils/cryptography/draft-EIP712.sol

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/draft-ERC20Permit.sol