code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L693-L697 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Sig.sol#L20-L31

Vulnerability details

Impact

The solidity function ecrecover is used, however the error result of 0 is not checked for. See documentation: https://docs.soliditylang.org/en/v0.8.9/units-and-global-variables.html?highlight=ecrecover#mathematical-and-cryptographic-functions “recover the address associated with the public key from elliptic curve signature or return zero on error. ”

Now you can supply invalid input parameters to the Sig.recover function, which will then result 0. If you also set o.maker to be 0 then this will match and an invalid signature is not detected.

So you can do all kinds of illegal & unexpected transactions.

This also found in previous audit: https://code4rena.com/reports/2021-09-swivel/#h-04-return-value-of-0-from-ecrecover-not-checked

Proof of Concept

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

  function validOrderHash(Hash.Order calldata o, Sig.Components calldata c) internal view returns (bytes32) {
    bytes32 hash = Hash.order(o);

    if (cancelled[hash]) { revert Exception(2, 0, 0, address(0), address(0)); }

    if (o.expiry < block.timestamp) { revert Exception(3, o.expiry, block.timestamp, address(0), address(0)); }

    address recovered = Sig.recover(Hash.message(domain, hash), c);

    if (o.maker != recovered) { revert Exception(4, 0, 0, o.maker, recovered); }

    return hash;
  }

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Sig.sol#L20-L31

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

You have said that "The suggestion would be to filter 0x00 makers from the orderbook? (which we do)" but I don't see where you do.

Tools Used

Recommended Mitigation Steps

Verify that the result from ecrecover isn’t 0

  function validOrderHash(Hash.Order calldata o, Sig.Components calldata c) internal view returns (bytes32) {
    bytes32 hash = Hash.order(o);

    if (cancelled[hash]) { revert Exception(2, 0, 0, address(0), address(0)); }

    if (o.expiry < block.timestamp) { revert Exception(3, o.expiry, block.timestamp, address(0), address(0)); }

    address recovered = Sig.recover(Hash.message(domain, hash), c);

    if (o.maker != recovered && o.maker != address(0)) { revert Exception(4, 0, 0, o.maker, recovered); }

    return hash;
  }

scaraven commented 2 years ago

Copying an issue from a previous audit which has been acknowledged and then disagreeing with the acknowledgement feels like they are trying to game the system.

JTraversa commented 2 years ago

Above is valid, but moreover this would require approvals from address(0) somehow. Since that wouldnt be a concern, the attack vector isnt really valid.

robrobbins commented 2 years ago

you could argue that, for QA sake - that method should revert. it is not a threat to our system as @JTraversa explained - any 0x0 is going to be rejected after. however, it's a simple issue to fix and test, so...

robrobbins commented 2 years ago

addressed: https://github.com/Swivel-Finance/gost/pull/410

bghughes commented 2 years ago

Copying an issue from a previous audit which has been acknowledged and then disagreeing with the acknowledgement feels like they are trying to game the system.

Yes I agree here. Downgrading to QA