code-423n4 / 2022-07-swivel-findings

0 stars 1 forks source link

Reliance on external protocols #119

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L739-L759

Vulnerability details

Reliance on external protocols

Impact

If the protocols were exploited and become insolvent, the principal might be in danger, leading to lost of fund. Although the likelyhood is small, once it happened, the outcome might be disasterous.

Proof of Concept

The redemption of the underlying primarily depend on the nomal working conditions of the protocols deposit funds to.

Swivel/Swivel.sol


  function withdraw(uint8 p, address u, address c, uint256 a) internal returns (bool) {
    // TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
      return ICompound(c).redeemUnderlying(a) == 0;
    } else if (p == uint8(Protocols.Yearn)) {
      // yearn vault api states that withdraw returns uint256
      return IYearn(c).withdraw(a) >= 0;
    } else if (p == uint8(Protocols.Aave)) {
      // Aave v2 docs state that withraw returns uint256
      // TODO explain the withdraw args
      return IAave(aaveAddr).withdraw(u, a, address(this)) >= 0;
    } else if (p == uint8(Protocols.Euler)) {
      // Euler withdraw is void
      // TODO explain the 0
      IEuler(c).withdraw(0, a);
      return true;
    } else {
      // we will allow protocol[0] to also function as a catchall for Erc4626
      return IErc4626(c).withdraw(a, address(this), address(this)) >= 0;
    }
  }

However, if the protocols were compromised and become insolvent, the security of the principal is in question. And these protocols have been through multiple attacks in the past. Since these external protocols, libraries and other dependences are upgraded once in a while, the possibility of introducing new vulnerbilities do exist. Also, existing bugs might be found and utilized.

Tools Used

Mannual analysis.

Recommended Mitigation Steps

Purchase some insurance for the underlying to protect against failure of external protocols not in control. For example Nexus Mutual, InsurAce.

JTraversa commented 2 years ago

This isnt really an "issue" and definitely should not be rewarded.

bghughes commented 2 years ago

This isnt really an "issue" and definitely should not be rewarded.

I agree - downgrading to QA as this is a systemic external risk. Should the underlying assets become insolvent (e.g. cTokens) the protocol would be at risk but this is very low odds and in some sense a feature of the system given Swivel sits on top of these protocols

bghughes commented 2 years ago

Grouping this with the warden’s QA report #123

bghughes commented 2 years ago

Duplicate of #123

0xean commented 2 years ago

Closing as invalid.