`initiate()` and `exit()` ERC777 reentrancy

[code423n4]

Lines of code

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

Vulnerability details

Impact

Allows attacker to sell or buy more tokens than what their counterpart has signed for.

Proof of Concept

If underlying token is ERC777 attacker (msg.sender) can take control of execution on Safe.transferFrom(uToken, o.maker, msg.sender, premiumFilled); and call initiate() again to sell more nTokens than what o.maker wants to buy within that order (assuming they have enough balance).

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

function initiateZcTokenFillingVaultInitiate(Hash.Order calldata o, uint256 a, Sig.Components calldata c) internal {
    bytes32 hash = validOrderHash(o, c);
    uint256 amount = a + filled[hash];

    if (amount > o.principal) { revert Exception(5, amount, o.principal, address(0), address(0)); }

    // TODO assign amount or keep the ADD?
    filled[hash] += a;

    IErc20 uToken = IErc20(o.underlying);

    uint256 premiumFilled = (a * o.premium) / o.principal;
    Safe.transferFrom(uToken, o.maker, msg.sender, premiumFilled);

    // transfer principal + fee in underlying to swivel (from sender)
    uint256 fee = premiumFilled / feenominators[0];
    Safe.transferFrom(uToken, msg.sender, address(this), (a + fee));

    IMarketPlace mPlace = IMarketPlace(marketPlace);
    address cTokenAddr = mPlace.cTokenAddress(o.protocol, o.underlying, o.maturity);

    // perform the actual deposit type transaction, specific to a protocol
    if(!deposit(o.protocol, o.underlying, cTokenAddr, a)) { revert Exception(6, 0, 0, address(0), address(0)); }

    // alert marketplace 
    if (!mPlace.custodialInitiate(o.protocol, o.underlying, o.maturity, msg.sender, o.maker, a)) { revert Exception(8, 0, 0, address(0), address(0)); }

    emit Initiate(o.key, hash, o.maker, o.vault, o.exit, msg.sender, a, premiumFilled);
  }

If underlying token is ERC777 attacker (msg.sender) can take control of execution on Safe.transferFrom(uToken, o.maker, msg.sender, principalFilled - a); and call exit() again to buy more nTokens than what o.maker wants to sell within that order (assuming they have enough balance).

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

  function exitZcTokenFillingZcTokenInitiate(Hash.Order calldata o, uint256 a, Sig.Components calldata c) internal {
    bytes32 hash = validOrderHash(o, c);
    uint256 amount = a + filled[hash];

    if (amount > o.premium) { revert Exception(5, amount, o.premium, address(0), address(0)); }

    // TODO assign amount or keep the ADD?
    filled[hash] += a;       

    IErc20 uToken = IErc20(o.underlying);

    uint256 principalFilled = (a * o.principal) / o.premium;
    // transfer underlying from initiating party to exiting party, minus the price the exit party pays for the exit (premium), and the fee.
    Safe.transferFrom(uToken, o.maker, msg.sender, principalFilled - a);

    // transfer fee in underlying to swivel
    uint256 fee = principalFilled / feenominators[1];

    Safe.transferFrom(uToken, msg.sender, address(this), fee);

    // alert marketplace
    if (!IMarketPlace(marketPlace).p2pZcTokenExchange(o.protocol, o.underlying, o.maturity, msg.sender, o.maker, principalFilled)) { revert Exception(11, 0, 0, address(0), address(0)); }

    emit Exit(o.key, hash, o.maker, o.vault, o.exit, msg.sender, a, principalFilled);
  }

If underlying token is ERC777 attacker (msg.sender) can take control of execution on Safe.transferFrom(uToken, o.maker, msg.sender, premiumFilled); and call exit() again to sell more nTokens than what o.maker wants to buy within that order (assuming they have enough balance).

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

  function exitVaultFillingVaultInitiate(Hash.Order calldata o, uint256 a, Sig.Components calldata c) internal {
    bytes32 hash = validOrderHash(o, c);
    uint256 amount = a + filled[hash];

    if (amount > o.principal) { revert Exception(5, amount, o.principal, address(0), address(0)); }

    // TODO assign amount or keep the ADD?
    filled[hash] += a;

    IErc20 uToken = IErc20(o.underlying);

    // transfer premium from maker to sender
    uint256 premiumFilled = (a * o.premium) / o.principal;
    Safe.transferFrom(uToken, o.maker, msg.sender, premiumFilled);

    uint256 fee = premiumFilled / feenominators[3];
    // transfer fee in underlying to swivel from sender
    Safe.transferFrom(uToken, msg.sender, address(this), fee);

    // transfer <a> notional from sender to maker
    if (!IMarketPlace(marketPlace).p2pVaultExchange(o.protocol, o.underlying, o.maturity, msg.sender, o.maker, a)) { revert Exception(12, 0, 0, address(0), address(0)); }

    emit Exit(o.key, hash, o.maker, o.vault, o.exit, msg.sender, a, premiumFilled);
  }

If underlying token is ERC777 attacker (msg.sender) can take control of execution on Safe.transfer(uToken, msg.sender, premiumFilled - fee); and call exit() again to sell more nTokens than what o.maker wants to buy within that order (assuming they have enough balance).

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

  function exitVaultFillingZcTokenExit(Hash.Order calldata o, uint256 a, Sig.Components calldata c) internal {
    bytes32 hash = validOrderHash(o, c);
    uint256 amount = a + filled[hash];

    if (amount > o.principal) { revert Exception(5, amount, o.principal, address(0), address(0)); }

    // TODO assign amount or keep the ADD?
    filled[hash] += a;

    // redeem underlying on Compound and burn cTokens
    IMarketPlace mPlace = IMarketPlace(marketPlace);
    address cTokenAddr = mPlace.cTokenAddress(o.protocol, o.underlying, o.maturity);

    if (!withdraw(o.protocol, o.underlying, cTokenAddr, a)) { revert Exception(7, 0, 0, address(0), address(0)); }

    IErc20 uToken = IErc20(o.underlying);
    // transfer principal-premium  back to fixed exit party now that the interest coupon and zcb have been redeemed
    uint256 premiumFilled = (a * o.premium) / o.principal;
    Safe.transfer(uToken, o.maker, a - premiumFilled);

    // transfer premium-fee to floating exit party
    uint256 fee = premiumFilled / feenominators[3];
    Safe.transfer(uToken, msg.sender, premiumFilled - fee);

    // burn zcTokens + nTokens from o.maker and msg.sender respectively
    if (!mPlace.custodialExit(o.protocol, o.underlying, o.maturity, o.maker, msg.sender, a)) { revert Exception(9, 0, 0, address(0), address(0)); }

    emit Exit(o.key, hash, o.maker, o.vault, o.exit, msg.sender, a, premiumFilled);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Add a non-reetrant modifier (from OpenZeppelin) to initiate() and exit().

[ghost]

Based on the example given, I don't think reentrancy is possible because the primary check (revert if amount > o.principal or amount > o.premium) and update (filled[hash] += a;) still occurs before any transfers are done so even if control is given back to the user, as long as the order isn't completely filled, the user can call the functions again.

[robrobbins]

agree with @STYJ. Though I could try creating a mock 777 implementation and trying to circumvent the guards in place. Labelling as maybe for now

[robrobbins]

we don't have any ERC777 in the protocol

[bghughes]

we don't have any ERC777 in the protocol

This makes sense to not support ERC777 and I believe this claim is a bit esoteric. That said, the recommendation to protect against reentrancy is sound and perhaps all transfer calls should be moved to the end of the function to follow the Check-Effects-Iteraction pattern.

Downgrading to QA.

[bghughes]

Warden note the existing use of the Safe library too

[0xean]

marking as duplicate of #143 wardens QA report.