Overlay Protocol contest - WatchPug's results

Handle

WatchPug

Vulnerability details

OverlayToken.sol#_transferBurn() and _transferMint() does not validate recipient != address(0). While in _transfer(), it validates the input and make sure recipient != address(0).

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/ovl/OverlayToken.sol#L268-L286

function _transferMint(
    address sender,
    address recipient,
    uint256 amount,
    uint256 minted
) internal {

    uint256 senderBalance = _balances[sender];

    require(senderBalance >= amount, "ERC20: transfer amount exceeds balance");

    unchecked { _balances[sender] = senderBalance - amount; }

    _balances[recipient] += amount + minted;

    emit Transfer(sender, recipient, amount);
    emit Transfer(address(0), recipient, minted);

}

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/ovl/OverlayToken.sol#L194-L212

function _transferBurn(
    address sender,
    address recipient,
    uint256 amount,
    uint256 burnt
) internal {

    uint256 senderBalance = _balances[sender];

    require(senderBalance >= amount + burnt, "OVL:balance<amount+burnt");

    unchecked { _balances[sender] = senderBalance - amount - burnt; }

    _balances[recipient] += amount;

    emit Transfer(sender, recipient, amount);
    emit Transfer(sender, address(0), burnt);

}

Recommendation

Consider adding input validations.

Handle

WatchPug

Vulnerability details

Based on the context, marginBurnRate should be able to be updated after deployment. However, there is no function to update it.

Recommendation

Change to:

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/mothership/OverlayV1Mothership.sol#L158-L166

function adjustGlobalParams(
    uint16 _fee,
    uint16 _feeBurnRate,
    address _feeTo,
    uint _marginBurnRate
) external onlyGovernor {
    fee = _fee;
    feeBurnRate = _feeBurnRate;
    feeTo = _feeTo;
    marginBurnRate = _marginBurnRate;
}

Or change marginBurnRate to immutable if it's not supposed to be updated later (for gas saving).

Handle

WatchPug

Vulnerability details

Unused events increase contract size and gas usage at deployment.

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/market/OverlayV1OI.sol#L8-L8

event log(string k , uint v);

log is unused.

Handle

WatchPug

Vulnerability details

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/collateral/OverlayV1OVLCollateral.sol#L308-L308

if (_userValueAdjusted > _userDebt) _userValueAdjusted -= _userDebt;

_userValueAdjusted -= _userDebt will never underflow.

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/collateral/OverlayV1OVLCollateral.sol#L339-L345

IOverlayV1Market(pos.market).exitOI(
            pos.isLong,
            _userOi,
            _userOiShares,
            _userCost < _userValueAdjusted ? _userValueAdjusted - _userCost : 0,
            _userCost < _userValueAdjusted ? 0 : _userCost - _userValueAdjusted
        );

_userValueAdjusted - _userCost and _userCost - _userValueAdjusted will never underflow.

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/collateral/OverlayV1OVLCollateral.sol#L320-L336

if (_userCost < _userValueAdjusted) {

    ovl.transferMint(
        msg.sender, 
        _userCost, 
        _userValueAdjusted - _userCost
    );

} else {

    ovl.transferBurn(
        msg.sender, 
        _userValueAdjusted, 
        _userCost - _userValueAdjusted
    );

}

_userValueAdjusted - _userCost and _userCost - _userValueAdjusted will never underflow.

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/collateral/OverlayV1OVLCollateral.sol#L308-L308

if (_userValueAdjusted > _userDebt) _userValueAdjusted -= _userDebt;

_userValueAdjusted -= _userDebt will never underflow.

Handle

WatchPug

Vulnerability details

Redundant code increase contract size and gas usage at deployment.

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/ovl/OverlayToken.sol#L17-L17

bytes32 public constant ADMIN_ROLE = 0x00;

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/ovl/OverlayToken.sol#L27-L35

constructor() {

    _setupRole(ADMIN_ROLE, msg.sender);
    _setupRole(MINTER_ROLE, msg.sender);
    _setupRole(BURNER_ROLE, msg.sender);
    _setRoleAdmin(MINTER_ROLE, ADMIN_ROLE);
    _setRoleAdmin(BURNER_ROLE, ADMIN_ROLE);

}

L17, the constant ADMIN_ROLE is redundant as DEFAULT_ADMIN_ROLE is already defined in AccessControl.

L29 can be changed to _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);

L32-33 are redundant as the role admin is default to 0x00 therefore, they can be removed.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/collateral/OverlayV1OVLCollateral.sol#L163-L164

ovl.burn(address(this), _feeBurn + _liqBurn);
ovl.transfer(_feeTo, _feeForward + _liqForward);

Use transferBurn can combine two external calls into one to save gas.

Handle

WatchPug

Vulnerability details

Every reason string takes at least 32 bytes.

Use short reason strings that fits in 32 bytes or it will become more expensive.

Instances include:

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/ovl/OverlayToken.sol#L131-L131

require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance");

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/ovl/OverlayToken.sol#L277-L277

require(senderBalance >= amount, "ERC20: transfer amount exceeds balance");

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/ovl/OverlayToken.sol#L310-L310

require(currentAllowance >= subtractedValue, "ERC20: decreased allowance below zero");

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/ovl/OverlayToken.sol#L322-L328

require(sender != address(0), "ERC20: transfer from the zero address");
require(recipient != address(0), "ERC20: transfer to the zero address");

_beforeTokenTransfer(sender, recipient, amount);

uint256 senderBalance = _balances[sender];
require(senderBalance >= amount, "ERC20: transfer amount exceeds balance");

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/ovl/OverlayToken.sol#L381-L386

require(account != address(0), "ERC20: burn from the zero address");

_beforeTokenTransfer(account, address(0), amount);

uint256 accountBalance = _balances[account];
require(accountBalance >= amount, "ERC20: burn amount exceeds balance");

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/ovl/OverlayToken.sol#L403-L404

require(owner != address(0), "ERC20: approve from the zero address");
require(spender != address(0), "ERC20: approve to the zero address");

Handle

WatchPug

Vulnerability details

Check of allowance can be done earlier to save some gas for failure transactions.

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/ovl/OverlayToken.sol#L118-L137

function transferFrom(
    address sender,
    address recipient,
    uint256 amount
) public virtual override returns (
    bool success_
) {

    _transfer(sender, recipient, amount);

    uint256 currentAllowance = _allowances[sender][_msgSender()];

    require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance");

    unchecked { _approve(sender, _msgSender(), currentAllowance - amount); }

    success_ = true;

}

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/ovl/OverlayToken.sol#L167-L186

function transferFromBurn(
    address sender,
    address recipient,
    uint256 amount,
    uint256 burnt
) public override onlyBurner returns (
    bool success
) {

    _transferBurn(sender, recipient, amount, burnt);

    uint256 currentAllowance = _allowances[sender][msg.sender];

    require(currentAllowance >= amount + burnt, "OVL:allowance<amount+burnt");

    unchecked { _approve(sender, msg.sender, currentAllowance - amount - burnt); }

    success = true;

}

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/ovl/OverlayToken.sol#L241-L260

function transferFromMint(
    address sender,
    address recipient,
    uint256 amount,
    uint256 minted
) public override onlyMinter returns (
    bool
) {

    _transferMint(sender, recipient, amount, minted);

    uint256 currentAllowance = _allowances[sender][msg.sender];

    require(currentAllowance >= amount, "OVL:allowance<amount");

    unchecked { _approve(sender, msg.sender, currentAllowance - amount); }

    return true;

}

Handle

WatchPug

Vulnerability details

Every call to an external contract costs a decent amount of gas. In OverlayV1OVLCollateral.sol, mothership.fee() can be cached as a storage variable and save ~21000 gas each time.

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/collateral/OverlayV1OVLCollateral.sol#L305-L305

uint _feeAmount = _userNotional.mulUp(mothership.fee());

Recommendation

• Add a storage variable in OverlayV1OVLCollateral.sol;
• Add a function updateFee()
• Call updateFee() after OverlayV1Mothership.sol#adjustGlobalParams()

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/OverlayV1UniswapV3Market.sol#L14-L14

uint256 internal X96 = 0x1000000000000000000000000;

Some storage variables include X96 will not never be changed and they should not be.

Changing them to constant can save gas.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/market/OverlayV1Market.sol#L21-L21

modifier lock() { require(unlocked == 1, "OVLV1:!unlocked"); unlocked = 0; _; unlocked = 1; }

SSTORE from 0 to 1 (or any non-zero value), the cost is 20000; SSTORE from 1 to 2 (or any other non-zero value), the cost is 5000.

By storing the original value once again, a refund is triggered (https://eips.ethereum.org/EIPS/eip-2200).

Since refunds are capped to a percentage of the total transaction's gas, it is best to keep them low, to increase the likelihood of the full refund coming into effect.

Therefore, switching between 1, 2 instead of 0, 1 will be more gas efficient.

See: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/86bd4d73896afcb35a205456e361436701823c7a/contracts/security/ReentrancyGuard.sol#L29-L33

Handle

WatchPug

Vulnerability details

For the storage variables that will be accessed multiple times, cache them in the stack can save ~100 gas from each extra read (SLOAD after Berlin).

For example:

• ovl in OverlayV1Mothership#enableCollateral()

  https://github.com/code-423n4/2021-11-overlay/blob/1833b792caf3eb8756b1ba5f50f9c2ce085e54d0/contracts/mothership/OverlayV1Mothership.sol#L133-L143

  function enableCollateral (address _collateral) external onlyGovernor {
  
      require(collateralExists[_collateral], "OVLV1:!exists");
  
      require(!collateralActive[_collateral], "OVLV1:!disabled");
  
      OverlayToken(ovl).grantRole(OverlayToken(ovl).MINTER_ROLE(), _collateral);
  
      OverlayToken(ovl).grantRole(OverlayToken(ovl).BURNER_ROLE(), _collateral);
  
  }

• ovl in OverlayV1Mothership#disableCollateral()

• ovl in OverlayV1Mothership#initializeCollateral()