QA Report - Githubissues

code423n4 commented 2 years ago

Missing invalid market check

https://github.com/code-423n4/2022-07-swivel/blob/main/Marketplace/MarketPlace.sol

Any functions that declare Market memory market = markets[p][u][m];

Should check whether market has a valid zctoken or not but now it don't check it which allow malicious market input.

require(market.zcToken != address(0), "Market not created");

We may not need signature to cancel order

Just check whether msg.sender == o[i].maker is enough to allow cancellation https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L407-L423

Change to this

  function cancel(Hash.Order[] calldata o) external returns (bool) {
    uint256 len = o.length;
    for (uint256 i; i < len;) {
      bytes32 hash = Hash.order(o[i]);
      if (msg.sender != o[i].maker) { revert Exception(15, 0, 0, msg.sender, o[i].maker); }

      cancelled[hash ] = true;

      emit Cancel(o[i].key, hash);

      unchecked {
        i++;
      }
    }

    return true;
  }

Exception is hard to read

revert Exception(30, 0, 0, address(0), address(0));

What is 30? Why not consider using separated custom errors?

error MatureMarketFailed();
...
revert MatureMarketFailed();

Variables name are hard to read

uint8 p, address u, uint256 m

Should be

uint8 penum, address underlying, uint256 maturity

robrobbins commented 2 years ago

on custom error codes

because 40+ custom errors are too many to add to a contract. their is / will be further clear documentation on what those error codes are.

missing market

i fail to see how it could be malicious

on variable names

disagree. first arguments are not variables. single letter tokens are reserved strictly for arguments in swivel (in all our codebases). they prevent the use of dangling underscores and help with the exhaustion-of-good-vars problem.

on cancel

maybe. will discuss this.