Overlay Protocol contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

There are TODOs left in the code. While this does not cause any direct issue, it indicates a bad smell and uncertainty. In previous reports, such submissions were assigned a score of 'low' so I think it's a fair game to submit this as an issue here also. Reference: https://github.com/code-423n4/2021-09-swivel-findings/issues/67 https://github.com/code-423n4/2021-10-tempus-findings/issues/39

Also, there are some misleading comments, e.g.:

    /// @notice Internal update function to price, cap, and pay funding.
    function update () public virtual returns (

the comment says that function is internal but it is actually declared as public.

Recommended Mitigation Steps

Consider implementing or removing TODOs and updating misleading comments.

Handle

pauliax

Vulnerability details

Impact

IOverlayV1Market declares a function adjustParams but OverlayV1Market does not have such a function. There may be more examples where interface and contract differ.

Recommended Mitigation Steps

Consider explicitly inheriting an interface to enforce compile-time check:

 contract OverlayV1Market is IOverlayV1Market

Handle

pauliax

Vulnerability details

Impact

Unused state variables or functions can be removed to save some deployment gas. Examples of such code are:

  uint16 public constant MIN_FEE = 1; // 0.01%
  uint16 public constant MAX_FEE = 100; // 1.00%
  uint16 public constant MIN_MARGIN_MAINTENANCE = 100; // 1% maintenance
  uint16 public constant MAX_MARGIN_MAINTENANCE = 6000; // 60% maintenance
  bytes32 public constant MINTER = keccak256("MINTER");
  bytes32 public constant BURNER = keccak256("BURNER");
  modifier onlyGuardian
  event log(string k, uint v);
  event log_addr(string k, address v);
  uint256 public leverageMax;

Handle

pauliax

Vulnerability details

Impact

There are places where the same storage variable is accessed multiple times in the same function. It would be more gas efficient to cache these variables and re-use them where necessary. E.g. functions initializeCollateral, enableCollateral, disableCollateral access ovl 4 times, function fetchPricePoint accesses macroWindow 4 times and microWindow 3 times. There are many more places where repeated storage access can be cached.

Handle

pauliax

Vulnerability details

Impact

Before:

  require(currentAllowance >= amount + burnt, "OVL:allowance<amount+burnt");
  unchecked { _approve(sender, msg.sender, currentAllowance - amount - burnt); }

After:

  uint totalAmount = amount + burnt;
  require(currentAllowance >= totalAmount, "OVL:allowance<amount+burnt");
  unchecked { _approve(sender, msg.sender, currentAllowance - totalAmount); }

This approach can be applied in several functions, e.g. transferFromBurn, _transferBurn.

Handle

pauliax

Vulnerability details

Impact

Can eliminate the subtraction here by substituting the order of operations:

  positions.push(Position.Info({
      market: _market,
      isLong: _isLong,
      leverage: _leverage,
      pricePoint: _pricePointNext,
      oiShares: 0,
      debt: 0,
      cost: 0
  }));

  positionId_ = positions.length - 1;

After:

  positionId_ = positions.length;
  positions.push...

Handle

pauliax

Vulnerability details

Impact

There are some structs that could be optimized for storage space. Struct variables are stored in 32 bytes each so you can group smaller types to occupy less storage, e.g. I don't think that leverage or pricePoint need to occupy uint256.

Consider using smaller types for values that are not expected to reach their limits. You can read more here: https://fravoll.github.io/solidity-patterns/tight_variable_packing.html or in the official documentation: https://docs.soliditylang.org/en/v0.4.21/miscellaneous.html

Handle

pauliax

Vulnerability details

Impact

Assigned operations to constant variables are re-evaluated every time. See https://github.com/ethereum/solidity/issues/9232

  bytes32 public constant GOVERNOR = keccak256("GOVERNOR");
  bytes32 public constant GUARDIAN = keccak256("GUARDIAN");
  bytes32 public constant MINTER = keccak256("MINTER");
  bytes32 public constant BURNER = keccak256("BURNER");

Recommended Mitigation Steps

Change 'constant' to 'immutable'.