code-423n4 / 2022-07-swivel-findings

0 stars 1 forks source link

Gas Optimizations #142

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Gas Optimizations

Finding Instances
[G-01] Implementing return is redundant if function already has a named returns method implemented 9
[G-02] marketPlace should be set in the constructor 1
[G-03] Using calldata instead of memory for read only arguments in `external functions saves gas 2
[G-04] uints smaller than uint256 cost more gas 2

Gas overview per contract

Contract Instances Gas Ops
ZcToken.sol 8 1
Swivel.sol 3 2
MarketPlace.sol 2 2
Creator.sol 1 1

Gas Optimizations

[G-01] Implementing return is redundant if function already has a named returns method implemented

Redundant return methods increase gas on deployment and execution. 9 instances of this issue have been found:

[G-01] MarketPlace.sol#L148-L168

  function authRedeem(uint8 p, address u, uint256 m, address f, address t, uint256 a) public authorized(markets[p][u][m].zcToken) returns (uint256 underlyingAmount) {
    Market memory market = markets[p][u][m];
    // if the market has not matured, mature it...
    if (market.maturityRate == 0) {
      if (!matureMarket(p, u, m)) { revert Exception(30, 0, 0, address(0), address(0)); }

      if (!IZcToken(market.zcToken).burn(f, a)) { revert Exception(29, 0, 0, address(0), address(0)); }

      ISwivel(swivel).authRedeem(p, u, market.cTokenAddr, t, a);

      return (a);
    } else {

      if (!IZcToken(market.zcToken).burn(f, a)) { revert Exception(29, 0, 0, address(0), address(0)); }

      uint256 amount = calculateReturn(p, u, m, a);
      ISwivel(swivel).authRedeem(p, u, market.cTokenAddr, t, amount);

      return (amount);
    }
  }
[G-01b] ZcToken.sol#L124-L137

    function redeem(uint256 principalAmount, address receiver, address holder) external override returns (uint256 underlyingAmount){
        // If maturity is not yet reached
        if (block.timestamp < maturity) { revert Maturity(maturity); }
        // some 5095 tokens may have custody of underlying and can can just burn PTs and transfer underlying out, while others rely on external custody
        if (holder == msg.sender) {
            return redeemer.authRedeem(protocol, underlying, maturity, msg.sender, receiver, principalAmount);
        }
        else {
            uint256 allowed = allowance[holder][msg.sender];
            if (allowed >= principalAmount) { revert Approvals(allowed, principalAmount); }
            allowance[holder][msg.sender] -= principalAmount;  
            return redeemer.authRedeem(protocol, underlying, maturity, holder, receiver, principalAmount);
        }
    }
[G-01c] ZcToken.sol#L98-L119

    function withdraw(uint256 underlyingAmount, address receiver, address holder) external override returns (uint256 principalAmount){
        uint256 previewAmount = this.previewWithdraw(underlyingAmount);
        // If maturity is not yet reached
        if (block.timestamp < maturity) {
            revert Maturity(maturity);
        }
        // Transfer logic
        // If holder is msg.sender, skip approval check
        if (holder == msg.sender) {
            redeemer.authRedeem(protocol, underlying, maturity, msg.sender, receiver, previewAmount);
            return previewAmount;
        }
        else {
            uint256 allowed = allowance[holder][msg.sender];
            if (allowed >= previewAmount) {
                revert Approvals(allowed, previewAmount);
            }
            allowance[holder][msg.sender] -= previewAmount;
            redeemer.authRedeem(protocol, underlying, maturity, holder, receiver, previewAmount); 
            return previewAmount;
        }
    }
[G-01d] ZcToken.sol#L87-L93

    function previewWithdraw(uint256 underlyingAmount) external override view returns (uint256 principalAmount){
        if (block.timestamp < maturity) {
            return 0;
        }
        return (underlyingAmount * IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate / IRedeemer(redeemer).getExchangeRate(protocol, cToken));
    }
[G-01e] ZcToken.sol#L79-L84

   function maxWithdraw(address owner) external override view returns (uint256 maxUnderlyingAmount){
        if (block.timestamp < maturity) {
            return 0;
        }
        return (balanceOf[owner] * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);
    }
[G-01f] ZcToken.sol#L70-L75

    function previewRedeem(uint256 principalAmount) external override view returns (uint256 underlyingAmount){
        if (block.timestamp < maturity) {
            return 0;
        }
        return (principalAmount * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);
    }
[G-01g] ZcToken.sol#L61-L65

    function maxRedeem(address owner) external override view returns (uint256 maxPrincipalAmount){
        if (block.timestamp < maturity) {
            return 0;
        }
        return (balanceOf[owner]);
[G-01h] ZcToken.sol#L52-L57

 function convertToPrincipal(uint256 underlyingAmount) external override view returns (uint256 principalAmount){
        if (block.timestamp < maturity) {
            return 0;
        }
        return (underlyingAmount * IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate / IRedeemer(redeemer).getExchangeRate(protocol, cToken));
    }
[G-01i] ZcToken.sol#L43-L48

function convertToUnderlying(uint256 principalAmount) external override view returns (uint256 underlyingAmount){
        if (block.timestamp < maturity) {
            return 0;
        }
        return (principalAmount * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);
    }

[G-02] marketPlace should be set in the constructor

setMarketPlace() can only be called once so making marketPlace immutable and setting it in the constructor would save gas. 1 instance of this issue has been found:

[G-02] Creator.sol#L54-L57

  function setMarketPlace(address m) external authorized(admin) returns (bool) {
    if (marketPlace != address(0)) {
      revert Exception(33, 0, 0, marketPlace, address(0)); 
    }

[G-03] Using calldata instead of memory for read only arguments in `external functions saves gas

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * mem_array.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one. 2 instances of this issue have been found:

[G-03] MarketPlace.sol#L64-L70

  function createMarket(
    uint8 p,
    uint256 m,
    address c,
    string memory n,
    string memory s
  ) external authorized(admin) unpaused(p) returns (bool) {
[G-03b] Swivel.sol#L495-L496

  function setFee(uint16[] memory i, uint16[] memory d) external authorized(admin) returns (bool) {

[G-04] uints smaller than uint256 cost more gas

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size. Solidity docs 2 instances of this issue have been found:

[G-04] Swivel.sol#L37-L38

uint16[4] public feenominators;

###### [G-04b] [Swivel.sol#L35-L36](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L35-L36)
```solidity

  uint16 constant public MIN_FEENOMINATOR = 33;
robrobbins commented 2 years ago

g2. incorrect, it cannot be due to deployment order

rest are dupes or wontfixes