Frankencoin - 0xnev's results

[L-01] Initialization period is a week but instead can be set to just 3 days

Position.sol#L53

/Position.sol
constructor(address _owner, address _hub, address _zchf, address _collateral, 
    uint256 _minCollateral, uint256 _initialLimit, uint256 initPeriod, uint256 _duration,
    uint256 _challengePeriod, uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) {
    require(initPeriod >= 3 days); // must be at least three days, recommended to use higher values
    setOwner(_owner);
    original = address(this);
    hub = _hub;
    price = _liqPrice;
    zchf = IFrankencoin(_zchf);
    collateral = IERC20(_collateral);
    mintingFeePPM = _mintingFeePPM;
    reserveContribution = _reservePPM;
    minimumCollateral = _minCollateral;
    challengePeriod = _challengePeriod;
    start = block.timestamp + initPeriod; // one week time to deny the position
    cooldown = start;
    expiration = start + _duration;
    limit = _initialLimit;
    
    emit PositionOpened(_owner, original, _zchf, address(collateral), _liqPrice);
}

In the frankencoin docs, it is stated that "Minting is not possible until the initialization period of seven days has passed."

However, in the constructor in Position.sol, it allows the initPeriod to be set at just a minimum of 3 days. Recommend to only allow initPeriod to be 7 days to be consistent to docs and code comments for start.

Recommendation:

constructor(address _owner, address _hub, address _zchf, address _collateral, 
    uint256 _minCollateral, uint256 _initialLimit, uint256 initPeriod, uint256 _duration,
    uint256 _challengePeriod, uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) {
    require(initPeriod >= 7 days); // must be at least three days, recommended to use higher values
    setOwner(_owner);
    original = address(this);
    hub = _hub;
    price = _liqPrice;
    zchf = IFrankencoin(_zchf);
    collateral = IERC20(_collateral);
    mintingFeePPM = _mintingFeePPM;
    reserveContribution = _reservePPM;
    minimumCollateral = _minCollateral;
    challengePeriod = _challengePeriod;
    start = block.timestamp + initPeriod; // one week time to deny the position
    cooldown = start;
    expiration = start + _duration;
    limit = _initialLimit;
    
    emit PositionOpened(_owner, original, _zchf, address(collateral), _liqPrice);
}

[G-01] unchecked block can be implemented for ERC20.transferFrom

Here, underflow is not possible due to check of currentAllowance < amount, so _approve can be wrapped with the unchecked block

ERC20.sol#L132

function transferFrom(address sender, address recipient, uint256 amount) external override returns (bool) {
    _transfer(sender, recipient, amount);
    uint256 currentAllowance = allowanceInternal(sender, msg.sender);
    if (currentAllowance < INFINITY){
        // Only decrease the allowance if it was not set to 'infinite'
        // Documented in /doc/infiniteallowance.md
        if (currentAllowance < amount) revert ERC20InsufficientAllowance(sender, currentAllowance, amount);
        _approve(sender, msg.sender, currentAllowance - amount);
    }
    return true;
}

Gas savings applies to all functions that calls ERC20.transferFrom(). For example, saves on average 11 gas for MintingHub.openPosition function that calls ERC20.transferFrom().

[G-02] unchecked block can be implemented for ERC20._transfer

Here, _balances[sender] -= amount will never underflow due to check of _balances[sender] < amount.

_balances[recipient] += amount will never overflow since sum of all balances are capped by totalSupply.

ERC20.sol#L156-L157

function _transfer(address sender, address recipient, uint256 amount) internal virtual {
   require(recipient != address(0));
   
   _beforeTokenTransfer(sender, recipient, amount);
   if (_balances[sender] < amount) revert ERC20InsufficientBalance(sender, _balances[sender], amount);
   _balances[sender] -= amount;
   _balances[recipient] += amount;
   emit Transfer(sender, recipient, amount);
}

Recommendation: Hence, _balances[sender] -= amount and _balances[recipient] += amount can be wrapped with the unchecked block.

Gas savings applies to all functions that calls ERC20._transfer(). For example, saves on average 238 gas for Frankencoin.suggestMinter that calls ERC20._transfer()

[G-03] unchecked block can be implemented for ERC20._mint

Here, overflow of _balances[recipient] += amount is not possible since it is at most _totalSupply + amount, which is checked automatically by solidity ^0.8.0.

ERC20.sol#L185

function _mint(address recipient, uint256 amount) internal virtual {
    require(recipient != address(0));

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

    _totalSupply += amount;
    _balances[recipient] += amount;
    emit Transfer(address(0), recipient, amount);
}

Recommendation: Hence, _balances[recipient] += amount can be wrapped with the unchecked block.

Gas savings applies to all functions that calls ERC20._mint(). For example, saves on average 242 gas for Frankencoin.mint(address,uint256,uint32,uint32) that calls ERC20._mint()

[G-04] unchecked block can be implemented for ERC20._burn

Here, underflow of _totalSupply -= amount is not possible since underflow is checked by _balances[account] -= amount and _totalSupply >= _balances[account] >= amount

ERC20.sol#L203-L204

function _burn(address account, uint256 amount) internal virtual {
    _beforeTokenTransfer(account, address(0), amount);

    _totalSupply -= amount;
    _balances[account] -= amount;
    emit Transfer(account, address(0), amount);
}

Recommendation: Hence we can refactor the function such that _totalSupply -= amount can be wrapped with the unchecked block

function _burn(address account, uint256 amount) internal virtual {
    _beforeTokenTransfer(account, address(0), amount);

    _balances[account] -= amount;
    unchecked{
        _totalSupply -= amount;
    }
    emit Transfer(account, address(0), amount);
}

Gas savings applies to all functions that calls ERC20._burn(). For example, saves on average 82 gas for Frankencoin.burnFrom() that calls ERC20._burn()

[G-05] Unchecked blocks when there is previous checks can be implemented

Unchecked block for Position.mintInternal()

Here, since limit is always greater than or equal to minted + amount, minted += amount; can be wrapped in an unchecked block.

Position.sol#L196

function mintInternal(address target, uint256 amount, uint256 collateral_) internal {
    if (minted + amount > limit) revert LimitExceeded();
    zchf.mint(target, amount, reserveContribution, calculateCurrentFee());
    minted += amount;

    checkCollateral(collateral_, price);
    emitUpdate();
}

Gas savings applies to all functions that calls Position.mintInternal(). For example, saves on average 67 gas for Position.mint() function that calls mintInternal()

Unchecked block for Position.notifyRepaidInternal()

Here, since minted is always greater than or equal to amount, minted -= amount; can be wrapped in an unchecked block

Position.sol#L242

function notifyRepaidInternal(uint256 amount) internal {
    if (amount > minted) revert RepaidTooMuch(amount - minted);
    minted -= amount;
}

Gas savings applies to all functions that calls Position.notifyRepaidInternal(). For example, saves on average 73 gas for Position.notifyChallengeSucceeded() function that calls Position.notifyRepaidInternal().

[G-06] State variables can be packed in fewer storage slots

Position.sol#L38-L39

2 results - 1 file

/Position.sol
32:    address public immutable original; // originals point to themselves, clone to their origin
33:    address public immutable hub; // the hub this position was created by
34:    IFrankencoin public immutable zchf; // currency
35:    IERC20 public override immutable collateral; // collateral
36:    uint256 public override immutable minimumCollateral; // prevent dust amounts
37:
38:    uint32 public immutable mintingFeePPM; /// @audit 4 bytes can be packed
39:    uint32 public immutable reserveContribution; /// @audit 4 bytes can be packed

Equity.sol#L39-L51

/Equity.sol

39:    uint32 public constant VALUATION_FACTOR = 3;
40:
41:    uint256 private constant MINIMUM_EQUITY = 1000 * ONE_DEC18;
42:
43:    /**
44:     * The quorum in basis points. 100 is 1%.
45:     */
46:    uint32 private constant QUORUM = 300;
47:
48:    /**
49:     * The number of digits to store the average holding time of share tokens.
50:     */
51:   uint8 private constant BLOCK_TIME_RESOLUTION_BITS = 24;

Recommendation: Saves 2 slots (4000 gas)

address public immutable original; // originals point to themselves, clone to their origin
uint32 public immutable mintingFeePPM;
address public immutable hub; // the hub this position was created by
uint32 public immutable reserveContribution; // in ppm
IFrankencoin public immutable zchf; // currency
IERC20 public override immutable collateral; // collateral
uint256 public override immutable minimumCollateral; // prevent dust amounts

uint32 public constant VALUATION_FACTOR = 3;

/**
    * The quorum in basis points. 100 is 1%.
    */
uint32 private constant QUORUM = 300;

/**
    * The number of digits to store the average holding time of share tokens.
    */
uint8 private constant BLOCK_TIME_RESOLUTION_BITS = 24;

uint256 private constant MINIMUM_EQUITY = 1000 * ONE_DEC18;

[G-07] Use nested if statements to avoid multiple check combinations using &&

Position.sol#L294

4 results - 2 files

/Position.sol
294:        if (size < minimumCollateral && size < collateralBalance()) revert ChallengeTooSmall();

Frankencoin.sol#L84-L85 Frankencoin.sol#L267

/Frankencoin.sol
84:      if (_applicationPeriod < MIN_APPLICATION_PERIOD && totalSupply() > 0) revert PeriodTooShort();

85:      if (_applicationFee < MIN_FEE  && totalSupply() > 0) revert FeeTooLow();

267:      if (!isMinter(msg.sender) && !isMinter(positions[msg.sender])) revert NotMinter();

Using nested if statements is cheaper than using && for multiple check combinations (saves ~6gas). Additionally, it improves readability of code and better coverage reports.

[G-08] Ownable.onlyOwner() modifier can be refactored

Ownable.requireOwner() can be removed by refactoring onlyOwner() modifier.

Ownable.sol#L45-L52

function requireOwner(address sender) internal view {
    if (owner != sender) revert NotOwner();
}

modifier onlyOwner() {
    requireOwner(msg.sender);
    _;
}

Recommendation:

modifier onlyOwner() {
    if (owner != msg.sender) revert NotOwner();
    _;
}

Gas savings applies to all functions that uses the onlyOwner() modifier. For example saves on average 76 gas for Position.adjust() function that uses onlyOwner() modifier.

[G-09] Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

Frankencoin.sol#L45 Frankencoin.sol#L50

/Frankencoin.sol
45:   mapping (address => uint256) public minters;

50:   mapping (address => address) public positions;

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot.