Frankencoin - MohammedRizwan's results

Low risk issues

Non-Critical issues

Low risk issues

[L‑01] For immutable variables, Zero address and zero value checks are missing in constructor

Zero address and zero value checks should be used in the constructors, to avoid the risk of setting a storage variable as zero address and zero value at the time of deployment. There are 10 instances of this issue.

In Position.sol contract constructor, Below are immutable variables.

File: contracts/Position.sol

24    uint256 public immutable challengePeriod; // challenge period in seconds
---

29    uint256 public immutable start; // timestamp when minting can start
30    uint256 public immutable expiration; // timestamp at which the position expires
31
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;
39    uint32 public immutable reserveContribution; // in ppm

Link to code

File: contracts/Position.sol

50    constructor(address _owner, address _hub, address _zchf, address _collateral, 
51        uint256 _minCollateral, uint256 _initialLimit, uint256 initPeriod, uint256 _duration,
52        uint256 _challengePeriod, uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) {
53        require(initPeriod >= 3 days); // must be at least three days, recommended to use higher values
54        setOwner(_owner);
55        original = address(this);
56        hub = _hub;
57        price = _liqPrice;
58        zchf = IFrankencoin(_zchf);
59        collateral = IERC20(_collateral);
60        mintingFeePPM = _mintingFeePPM;
61        reserveContribution = _reservePPM;
62        minimumCollateral = _minCollateral;
63        challengePeriod = _challengePeriod;
64        start = block.timestamp + initPeriod; // one week time to deny the position
65        cooldown = start;
66        expiration = start + _duration;
67        limit = _initialLimit;
68        
69        emit PositionOpened(_owner, original, _zchf, address(collateral), _liqPrice);
70    }

Link to code

Recommended Mitigation Steps:

Immutable variables once set during deployment can not be changed, It is better not to take risk and do the address and value validation so that error should not happen. Below is the recommended code.

File: contracts/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(_hub != address(0), "invalid address");
+       require(_zchf != address(0), "invalid address");
+       require(_collateral != address(0), "invalid address");
+       require(_minCollateral != 0, "invalid amount");
+       require(_challengePeriod != 0, "invalid period");
+       require(_mintingFeePPM != 0, "invalid fee");
+       require(_reservePPM != 0, "invalid amount");
        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 MintingHub.sol contract constructor, Below are immutable variables.

File: contracts/MintingHub.sol

28    IPositionFactory private immutable POSITION_FACTORY; // position contract to clone
---

30    IFrankencoin public immutable zchf; // currency

Link to code

File: contracts/MintingHub.sol

54    constructor(address _zchf, address factory) {
55        zchf = IFrankencoin(_zchf);
56        POSITION_FACTORY = IPositionFactory(factory);
57    }

Link to code

Recommended Mitigation Steps:

Immutable variables once set during deployment can not be changed, It is better not to take risk and do the address and value validation so that error should not happen. Below is the recommended code.

File: contracts/MintingHub.sol

    constructor(address _zchf, address factory) {
+       require(_zchf != address(0), "invalid address");
+       require(factory != address(0), "invalid address");
        zchf = IFrankencoin(_zchf);
        POSITION_FACTORY = IPositionFactory(factory);
    }

In Frankencoin.sol contract constructor, Below are immutable variables.

File: contracts/Frankencoin.sol

26   uint256 public immutable MIN_APPLICATION_PERIOD; // for example 10 days
---

31   IReserve override public immutable reserve;

Link to code

File: contracts/Frankencoin.sol

59   constructor(uint256 _minApplicationPeriod) ERC20(18){
60      MIN_APPLICATION_PERIOD = _minApplicationPeriod;
61      reserve = new Equity(this);
62   }

Link to code

Recommended Mitigation steps

File: contracts/Frankencoin.sol

   constructor(uint256 _minApplicationPeriod) ERC20(18){
+     require(_minApplicationPeriod != 0, "invalid period");
      MIN_APPLICATION_PERIOD = _minApplicationPeriod;
      reserve = new Equity(this);
   }

[L‑02] Withdraw to zero address should be avoided

Withdraw collateral or tokens must be avoided to address(0) as if mistakenly sent to address(0), it can not be recovered. withdraw() can be accessed by onlyOwner but address(0) transfer can not be rule out as onlyOwner can also transfer by mistake to address(0). There is 1 instance of this issue.

File: contracts/Position.sol

249    function withdraw(address token, address target, uint256 amount) external onlyOwner {
250        if (token == address(collateral)){
251            withdrawCollateral(target, amount);
252        } else {
253            IERC20(token).transfer(target, amount);
254        }
255    }

Link to code

Recommended Mitigation steps

Add zero address check validation in withdraw function to prevent this issue.

File: contracts/Position.sol

    function withdraw(address token, address target, uint256 amount) external onlyOwner {
+       require(target != address(0), "invalid address");
        if (token == address(collateral)){
            withdrawCollateral(target, amount);
        } else {
            IERC20(token).transfer(target, amount);
        }
    }

Non-Critical issues

[N‑01] For internal functions, follow Solidity standard naming conventions

Proper use of _ as a function name prefix should be taken care and a common pattern is to prefix internal and private function names with _. This is partially incorporated in contracts. Below internal functions does not follow this pattern which leads to confusion while reading code and affects overall readability of the code.

There are 21 instances of this issue:

File: contracts/Position.sol

169    function collateralBalance() internal view returns (uint256){

Link to code

File: contracts/Position.sol

193    function mintInternal(address target, uint256 amount, uint256 collateral_) internal {

Link to code

File: contracts/Position.sol

202    function restrictMinting(uint256 period) internal {

Link to code

File: contracts/Position.sol

232    function repayInternal(uint256 burnable) internal {

Link to code

File: contracts/Position.sol

240    function notifyRepaidInternal(uint256 amount) internal {

Link to code

File: contracts/Position.sol

268    function internalWithdrawCollateral(address target, uint256 amount) internal returns (uint256) {

Link to code

File: contracts/Position.sol

282    function checkCollateral(uint256 collateralReserve, uint256 atPrice) internal view {

Link to code

File: contracts/Position.sol

286    function emitUpdate() internal {

Link to code

File: contracts/MintingHub.sol

188    function minBid(Challenge storage challenge) internal view returns (uint256) {

Link to code

File: contracts/MintingHub.sol

287    function returnCollateral(Challenge storage challenge, bool postpone) internal {

Link to code

File: contracts/Equity.sol

144    function adjustTotalVotes(address from, uint256 amount, uint256 roundingLoss) internal {

Link to code

File: contracts/Equity.sol

157    function adjustRecipientVoteAnchor(address to, uint256 amount) internal returns (uint256){

Link to code

File: contracts/Equity.sol

172    function anchorTime() internal view returns (uint64){

Link to code

File: contracts/Equity.sol

225    function canVoteFor(address delegate, address owner) internal view returns (bool) {

Link to code

File: contracts/Equity.sol

266    function calculateSharesInternal(uint256 capitalBefore, uint256 investment) internal view returns (uint256) {

Link to code

File: contracts/Frankencoin.sol

102   function allowanceInternal(address owner, address spender) internal view override returns (uint256) {

Link to code

File: contracts/ERC20.sol

97    function allowanceInternal(address owner, address spender) internal view virtual returns (uint256) {

Link to code

File: contracts/Ownable.sol

39    function setOwner(address newOwner) internal {

Link to code

File: contracts/PositionFactory.sol

37    function createClone(address target) internal returns (address result) {

Link to code

File: contracts/StablecoinBridge.sol

49    function mintInternal(address target, uint256 amount) internal {

Link to code

File: contracts/StablecoinBridge.sol

67    function burnInternal(address zchfHolder, address target, uint256 amount) internal {

Link to code

[N‑02] Solidity compiler version should be exactly same in all smart contracts

Solidity compiler version should be exactly same in all smart contracts. Different Solidity compiler versions are used, the following contracts have mix versions.

There are 10 instances of this issue.

File: contracts/Position.sol

2    pragma solidity ^0.8.0;

Link to code

File: contracts/Equity.sol

4    pragma solidity >=0.8.0 <0.9.0;

Link to code

File: contracts/ERC20.sol

14   pragma solidity ^0.8.0;

Link to code

File: contracts/MathUtil.sol

3    pragma solidity >=0.8.0 <0.9.0;

Link to code

File: contracts/ERC20PermitLight.sol

5    pragma solidity ^0.8.0;

Link to code

File: contracts/Frankencoin.sol

2    pragma solidity ^0.8.0;

Link to code

File: contracts/MintingHub.sol

2    pragma solidity ^0.8.0;

Link to code

File: contracts/StablecoinBridge.sol

2    pragma solidity ^0.8.0;

Link to code

File: contracts/PositionFactory.sol

2    pragma solidity ^0.8.0;

Link to code

File: contracts/Ownable.sol

9    pragma solidity ^0.8.0;

Link to code

Recommendation Mitigation steps

Versions must be consistent with each other.

[N‑03] Use named parameters for mapping type declarations

Consider using named parameters in mappings (e.g. mapping(address account => uint256 balance)) to improve readability. This feature is present since Solidity 0.8.18. There are 8 instances of this issue.

File: contracts/MintingHub.sol

37    mapping (address /** col */ => mapping (address => uint256)) public pendingReturns;

Link to code

File: contracts/Equity.sol

83    mapping (address => address) public delegates;
---

88    mapping (address => uint64) private voteAnchor; // 40 Bit for the block number, 24 Bit sub-block time resolution

Link to code

File: contracts/Frankencoin.sol

45   mapping (address => uint256) public minters;
---

50   mapping (address => address) public positions;

Link to code

File: contracts/ERC20.sol

43    mapping (address => uint256) private _balances;
44
45    mapping (address => mapping (address => uint256)) private _allowances;

Link to code

File: contracts/ERC20PermitLight.sol

15    mapping(address => uint256) public nonces;

Link to code

[N‑04] Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

While the compiler knows to optimize away the exponentiation, it’s still better coding practice to use idioms that do not require compiler optimization, if they exist There are 4 instances of this issue:

File: contracts/Equity.sol

253        require(totalSupply() < 2**128, "total supply exceeded");

Link to code

File: contracts/Frankencoin.sol

25     uint256 public constant MIN_FEE = 1000 * (10**18);

Link to code

File: contracts/MathUtil.sol

10    uint256 internal constant ONE_DEC18 = 10**18;

Link to code

File: contracts/MintingHub.sol

20    uint256 public constant OPENING_FEE = 1000 * 10**18;

Link to code

Summary

Gas Optimizations

[G‑01] Avoid using state variable in emit

While emitting events, Passing parameter variables will cost less as compared to passing the storage values. There are 1 instances of this issues.

File: contracts/Position.sol

50    constructor(address _owner, address _hub, address _zchf, address _collateral, 
51        uint256 _minCollateral, uint256 _initialLimit, uint256 initPeriod, uint256 _duration,
52        uint256 _challengePeriod, uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) {
53        require(initPeriod >= 3 days); // must be at least three days, recommended to use higher values
54        setOwner(_owner);
55        original = address(this);
56        hub = _hub;
57        price = _liqPrice;
58        zchf = IFrankencoin(_zchf);
59        collateral = IERC20(_collateral);
60        mintingFeePPM = _mintingFeePPM;
61        reserveContribution = _reservePPM;
62        minimumCollateral = _minCollateral;
63        challengePeriod = _challengePeriod;
64        start = block.timestamp + initPeriod; // one week time to deny the position
65        cooldown = start;
66        expiration = start + _duration;
67        limit = _initialLimit;
68        
69        emit PositionOpened(_owner, original, _zchf, address(collateral), _liqPrice);
70    }

Link to code

Recommended Mitigation steps

By incorporating below recommendation, Some gas can be saved.

File: contracts/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);
+        emit PositionOpened(_owner, original, _zchf, _collateral, _liqPrice);
    }

[G‑02] Use nested if and, avoid multiple check combinations

Using nested if is cheaper than using && multiple check combinations. There are more advantages, such as easier to read code and better coverage reports. There are 4 instances of this issue.

File: contracts/Position.sol

294        if (size < minimumCollateral && size < collateralBalance()) revert ChallengeTooSmall();

Link to code

File: contracts/Frankencoin.sol

84      if (_applicationPeriod < MIN_APPLICATION_PERIOD && totalSupply() > 0) revert PeriodTooShort();
85      if (_applicationFee < MIN_FEE  && totalSupply() > 0) revert FeeTooLow();

Link to code

File: contracts/Frankencoin.sol

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

Link to code

[G‑03] Zero value check in mint function, can save gas for minter

mint() does not have zero value check which can waste lots of gas while called by minter for minting. It is recommended to have zero value check to prevent the wastage of gas. There is 1 instance of this issue.

File: contracts/Frankencoin.sol

165   function mint(address _target, uint256 _amount, uint32 _reservePPM, uint32 _feesPPM) override external 
      minterOnly {
166      uint256 usableMint = (_amount * (1000_000 - _feesPPM - _reservePPM)) / 1000_000; // rounding down is 
         fine
167      _mint(_target, usableMint);
168      _mint(address(reserve), _amount - usableMint); // rest goes to equity as reserves or as fees
169      minterReserveE6 += _amount * _reservePPM; // minter reserve must be kept accurately in order to ensure 
          we can get back to exactly 0
170   }

Link to code

Recommended Mitigation steps

Add zero value check in mint() function

File: contracts/Frankencoin.sol

   function mint(address _target, uint256 _amount, uint32 _reservePPM, uint32 _feesPPM) override external 
      minterOnly {
+     require(_amount != 0, "invalid amount");
      uint256 usableMint = (_amount * (1000_000 - _feesPPM - _reservePPM)) / 1000_000; // rounding down is fine
      _mint(_target, usableMint);
      _mint(address(reserve), _amount - usableMint); // rest goes to equity as reserves or as fees
      minterReserveE6 += _amount * _reservePPM; // minter reserve must be kept accurately in order to ensure we can get back to exactly 0
   }