Yearn vault integration is broken

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/fbf94f87994d91dce75c605a1822ec6d6d7e9e74/Marketplace/Compounding.sol#L55-L56 https://github.com/code-423n4/2022-07-swivel/blob/fbf94f87994d91dce75c605a1822ec6d6d7e9e74/Marketplace/MarketPlace.sol#L64-L73

Vulnerability details

Impact

Yearn integration is broken, making it impossible to create a MarketPlace associated with a Yearn vault.

Proof of Concept

Compounding.sol attempts to resolve the underlying token implemented by supported protocols:

https://github.com/code-423n4/2022-07-swivel/blob/fbf94f87994d91dce75c605a1822ec6d6d7e9e74/Marketplace/Compounding.sol#L50-L64

function underlying(uint8 p, address c) internal view returns (address) {
    if (p == uint8(Protocols.Compound)) {
      return ICompoundToken(c).underlying();
    } else if (p == uint8(Protocols.Rari)) {
      return ICompoundToken(c).underlying();
    } else if (p == uint8(Protocols.Yearn)) {
      return IYearnVault(c).underlying();
    } else if (p == uint8(Protocols.Aave)) {
      return IAaveToken(c).UNDERLYING_ASSET_ADDRESS();
    } else if (p == uint8(Protocols.Euler)) {
      return IEulerToken(c).underlyingAsset();
    } else {
      return IErc4626(c).asset();      
    }
  }

As can be seen in the excerpt above, the contract will attempt to access IYearnVault(c).underlying(); when integrating with a Yearn vault. Problem is this property is not present in Yearn vaults. Therefore this operation will always revert.

In particular, this would not allow to create a MarketPlace associated with a Yearn vault even though the protocol is stated as supported.

https://github.com/code-423n4/2022-07-swivel/blob/fbf94f87994d91dce75c605a1822ec6d6d7e9e74/Marketplace/MarketPlace.sol#L64-L73

function createMarket(
  uint8 p,
  uint256 m,
  address c,
  string memory n,
  string memory s
) external authorized(admin) unpaused(p) returns (bool) {
  if (swivel == address(0)) { revert Exception(21, 0, 0, address(0), address(0)); }

  address underAddr = Compounding.underlying(p, c);

Correct underlying token address in Yearn vaults is stored in the token property.

https://github.com/yearn/yearn-vaults/blob/beff27908bb2ae017ed73b773181b9b93f7435ad/contracts/Vault.vy#L74

Tools Used

vim

Recommended Mitigation Steps

Access the underlying token of a Yearn vault through the token property instead of the non-existent underlying which will cause a revert and the inability to create marketplaces associated with the protocol.

[JTraversa]

Given no funds would be at risk as this is an issue that would exist at the time of admin's market creation, i'd probably say that this issue is likely ~low or so

[bghughes]

Given no funds would be at risk as this is an issue that would exist at the time of admin's market creation, i'd probably say that this issue is likely ~low or so

Agreed that funds are not necessarily at risk but this is a broken functionality of the protocol. Given:

There are a few primary targets for concern:

1. Ensuring the new Compounding.sol library properly calculates the exchangeRate for each external protocol.
2. Ensuring the new withdraw and deposit methods on Swivel.sol properly encapsulate external protocol interactions.

I believe this should be a Medium Risk issue.

[robrobbins]

lol... market forces at work

[robrobbins]

the example given is incorrect. Compounding.Underlying() is correct as Compounding is our library that abstracts the actual call to get the asset in question.

the actual error is in the Compounding lib where .Protocol resolves to Protocols.Yearn

[robrobbins]

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