code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

Deposit isn't check for minimum share token received (In case of slippage).

Proof of Concept

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

  function deposit(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).mint(a) == 0;
    } else if (p == uint8(Protocols.Yearn)) {
      // yearn vault api states that deposit returns shares as uint256
      return IYearn(c).deposit(a) >= 0;
    } else if (p == uint8(Protocols.Aave)) {
      // Aave deposit is void. NOTE the change in pattern here where our interface is not wrapping a compounding token directly, but
      // a specified protocol contract whose address we have set
      // TODO explain the Aave deposit args
      IAave(aaveAddr).deposit(u, a, address(this), 0);
      return true;
    } else if (p == uint8(Protocols.Euler)) {
      // Euler deposit is void.
      // TODO explain the 0 (primary account)
      IEuler(c).deposit(0, a);
      return true;
    } else {
      // we will allow protocol[0] to also function as a catchall for Erc4626
      // NOTE: deposit, as per the spec, returns 'shares' but it is unknown if 0 would revert, thus we'll check for 0 or greater
      return IErc4626(c).deposit(a, address(this)) >= 0;
    }
  }

It only check that fund is deposited successfully but doesn't check that the received share token is enough or not. If one of this protocol can be sandwiched, the fund will be lost to the MEV bot.

Tools Used

Manual review

Recommended Mitigation Steps

Add share token address and minimum share received to the input and perform check at the end to require different in balance to be greater than minimum share.

JTraversa commented 2 years ago

Duplicate of #168

bghughes commented 2 years ago

Duplicate of #168

Note that this is not an exact duplicate IMO as they both reference the same idea in two different functions. That said, I don't think this is a very strong issue given that the external calls seemingly don't care about the share amount received. I believe the protocol moreso intends for the deposit to simply succeed or not, which the current implementation does check.

Given this, I'll downgrade to QA/informational for this warden and not mark it as a duplicate. Generally, the warden's recommendation is not needed at all.