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

Impact

Withdraw doesn't check for the minimum amount of underlying token withdrawn. If some protocol can be attacked by MEV sandwiching bot, user may loss fund to MEV bot on withdrawing.

Proof of Concept

  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;
    }
  }

Withdraw just check whether withdrawing is success or not. But not checking the final underlying token balance after withdraw to see if it is sandwiched or not.

Tools Used

Manual review

Recommended Mitigation Stepp

Add minimum underlying token received after withdraw to the function input. And check the different in underlying token balance at the end.

JTraversa commented 2 years ago

Interest bearing vaults cant be frontrun or sandwiched. You deposit/withdraw exact amounts in these cases

bghughes commented 2 years ago

Interest bearing vaults cant be frontrun or sandwiched. You deposit/withdraw exact amounts in these cases

Technically, they can be easily frontran via a priority gas auction taking place on L1. That said, as you intuitively understand, it is generally a moot point with over-collateralized lending vault tokens as returns are continuous rather than discrete.

This one is very similar to the comment I made here (https://github.com/code-423n4/2022-07-swivel-findings/issues/167#issuecomment-1200490937) and is QA at best. Grouping with the warden's main QA report at #165

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