Incorrect check for signature malleability

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Sig.sol#L26

Vulnerability details

Impact

Sig.recover() has an Incorrect check: c.v != 27 || c.v != 28. Thus, Sig.recover() always reverts.

Proof of Concept

c.v != 27 || c.v != 28 is always true https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Sig.sol#L26

  function recover(bytes32 h, Components calldata c) internal pure returns (address) {
    // EIP-2 and malleable signatures...
    // see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/cryptography/ECDSA.sol
    if (uint256(c.s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
      revert S();
    }
    if(c.v != 27 || c.v != 28) {
      revert V();
    }

    return ecrecover(h, c.v, c.r, c.s);
  }

Tools Used

None

Recommended Mitigation Steps

Fix the check.

  function recover(bytes32 h, Components calldata c) internal pure returns (address) {
    // EIP-2 and malleable signatures...
    // see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/cryptography/ECDSA.sol
    if (uint256(c.s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
      revert S();
    }
    if(c.v != 27 && c.v != 28) {
      revert V();
    }

    return ecrecover(h, c.v, c.r, c.s);
  }

[JTraversa]

Id say that this should probably be downgraded to ~low or maybe medium because:

1. There arent funds at risk, it would just prevent initialization of the protocol.
2. Scope limitations

[robrobbins]

no PR for a fix on this as it was a regression that was corrected on an initial branch cleanup

https://github.com/Swivel-Finance/gost/blob/v3/test/swivel/Sig.sol#L28

[bghughes]

I believe this is a good issue given this low-level error breaks validOrderHash's expected functionality potentially allowing someone to spoof order activities. It may seem small, but allowing for fake orders could cause edge cases in which market participants are robbed over time.

[JTraversa]

There would not be any fake orders, the protocol would not function whatsoever.

That said, I'm going off of the severity definitions provided by code4rena solely, which describes high risk as, "Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals)."

Id say that there is clearly no risk of stolen, lost, or compromised assets here, hence the severity dispute (it should probably alongside other similar issues as either low/medium e.g. the Yearn integration that would not have initialized correctly)

[0xean]

Per the readme

So the only things explicitly NOT in scope:
- Details of 5095 implementation (the EIP isnt final yet)
- A few external Libs: 
        - FixedPointMathLib
        - LibCompound
        - LibFuse
        - Safe.sol
- Some older stuff not worth a review:
        - Hash.sol
        - Sig.sol

Sponsors have the ability to declare what is in scope per C4, so this issue is being marked invalid to keep the contest fair to all wardens. That being said, it would be a good gesture of the Sponsor to maybe tip the warden as the issue is valid in the posted code.