Frankencoin - nadin's results

Non-Critical Issues List

Issue

[N-01] Undocumented assembly blocks ( 01 instances )

[N-02] It's better to emit after all processing is done ( 07 instances)

[N-03] Duplicate constant ( 01 instances )

[N-04] Take advantage of Custom Error's return value property ( 09 instances )

[N-05] Include the project name and development team information in the contract to increase the popularity and trust of users in the project

[N-06] Project Upgrade and Stop Scenario should be

[N-07] Use SMTChecker

[N-08] Solidity compiler optimizations can be problematic

[N-09] Showing the actual values of numbers in NatSpec comments makes checking and reading code easier ( 04 instances )

Total: 22 instances over 09 issues

[N-01] Undocumented assembly blocks

Description:

In PositionFactory.sol includes a function createClone with an assembly block. Assembly is a low-level language that is harder to parse by readers. Consider including extensive inline documentation clearly explaining what every single assembly instruction does. This will make it easier for users to trust the code, for reviewers to verify it, and for developers to build on top of it or update it. Note that the use of assembly discards several important safety features of Solidity, which may render the code less safe and more error-prone. Hence, consider implementing thorough tests to cover all potential use cases of the createClone function to ensure it behaves as expected.

Recommendation:

/* copied from https://github.com/optionality/clone-factory/blob/32782f82dfc5a00d103a7e61a17a5dedbd1e8e9d/contracts/CloneFactory.sol
 + Template Code for the create clone method:
+  function createClone(address target) internal returns (address result) {
+    bytes20 targetBytes = bytes20(target)${bytes == 20 ? "" : "<<" + ((20 - bytes) * 8)};
+    assembly {
+      let clone := mload(0x40)
+      mstore(clone, 0x${code.substring(0, 2*(cloner.labels.address + 1)).padEnd(64, '0')})
+      mstore(add(clone, 0x${(cloner.labels.address + 1).toString(16)}), targetBytes)
+      mstore(add(clone, 0x${(cloner.labels.address + bytes + 1).toString(16)}), 0x${code.substring(2*(cloner.labels.address + bytes + 1), 2*(cloner.labels.address+bytes+1) + 30).padEnd(64, '0')})
+      result := create(0, clone, 0x${(code.length / 2).toString(16)})
+    }
+  }
+ */
function createClone(address target) internal returns (address result) {
+        // convert address to bytes20 for assembly use
        bytes20 targetBytes = bytes20(target);
        assembly {
+            // allocate clone memory
            let clone := mload(0x40)
+            // store initial portion of the delegation contract code in bytes form
            mstore(clone, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000000000000000000000)
+            // store the provided address
            mstore(add(clone, 0x14), targetBytes)
+            // store the remaining delegation contract code
            mstore(add(clone, 0x28), 0x5af43d82803e903d91602b57fd5bf30000000000000000000000000000000000)
+            // create the actual delegate contract reference and return its address
            result := create(0, clone, 0x37)
        }
    }

[N-02] It's better to emit after all processing is done ( 07 instances)

File: contracts/Equity.sol
249:        emit Trade(msg.sender, int(shares), amount, price());
280:        emit Trade(msg.sender, -int(shares), proceeds, price());

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L249 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L280

File: contracts/MintingHub.sol
146:        emit ChallengeStarted(msg.sender, address(position), _collateralAmount, pos);
176:        emit ChallengeStarted(challenge.challenger, address(challenge.position), challenge.size, _challengeNumber);
177:        emit ChallengeStarted(copy.challenger, address(copy.position), copy.size, pos);
212:            emit ChallengeAverted(address(challenge.position), _challengeNumber);
274:        emit ChallengeSucceeded(address(challenge.position), challenge.bid, _challengeNumber);

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L274

[N-03] Duplicate constant

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

https://github.com/code-423n4/2023-04-frankencoin/tree/main/contracts/Frankencoin.sol#L25

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

https://github.com/code-423n4/2023-04-frankencoin/tree/main/contracts/MintingHub.sol#L20

[N-04] Take advantage of Custom Error's return value property ( 09 instances )

An important feature of Custom Error is that values such as address, tokenID, msg.value can be written inside the () sign, this kind of approach provides a serious advantage in debugging and examining the revert details of dapps such as tenderly.

File: contracts/Position.sol
77:        if(_coll < minimumCollateral) revert InsufficientCollateral();
81:        if (price > _price) revert InsufficientCollateral();
201:        if (block.timestamp >= challenge.end) revert TooLate();
202:        if (expectedSize != challenge.size) revert UnexpectedSize();

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L201-L202

File: contracts/Equity.sol
211:        if (_votes * 10000 < QUORUM * totalVotes()) revert NotQualified();

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L211

File: contracts/Frankencoin.sol
84:      if (_applicationPeriod < MIN_APPLICATION_PERIOD && totalSupply() > 0) revert PeriodTooShort();
85:      if (_applicationFee < MIN_FEE  && totalSupply() > 0) revert FeeTooLow();
86:      if (minters[_minter] != 0) revert AlreadyRegistered();
126:      if (!isMinter(msg.sender)) revert NotMinter();
153:      if (block.timestamp > minters[_minter]) revert TooLate();

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L153

[N-05] Include the project name and development team information in the contract to increase the popularity and trust of users in the project

Recommendation: Use form like FraxFinance project

https://github.com/FraxFinance/frxETH-public/blob/7f7731dbc93154131aba6e741b6116da05b25662/src/sfrxETH.sol#L4-L24

[N-06] Project Upgrade and Stop Scenario should be

At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an " EMERGENCY STOP (CIRCUIT BREAKER) PATTERN ".

https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol

[N-07] Use SMTChecker

The highest tier of smart contract behavior assurance is formal mathematical verification. All assertions that are made are guaranteed to be true across all inputs → The quality of your asserts is the quality of your verification.

https://twitter.com/0xOwenThurm/status/1614359896350425088?t=dbG9gHFigBX85Rv29lOjIQ&s=19

[N-08] Solidity compiler optimizations can be problematic

96:          optimizer: {
97:              enabled: true,
98:              runs: 200
99:              },

Link to code

Description: Protocol has enabled optional compiler optimizations in Solidity. There have been several optimization bugs with security implications. Moreover, optimizations are actively being developed. Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them.

Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past. A high-severity bug in the emscripten-generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG.

Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. More recently, another bug due to the incorrect caching of keccak256 was reported. A compiler audit of Solidity from November 2018 concluded that the optional optimizations may not be safe. It is likely that there are latent bugs related to optimization and that new bugs will be introduced due to future optimizations.

Exploit Scenario A latent or future bug in Solidity compiler optimizations—or in the Emscripten transpilation to solc-js—causes a security vulnerability in the contracts.

Recommendation: Short term, measure the gas savings from optimizations and carefully weigh them against the possibility of an optimization-related bug. Long term, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

[N-09] Showing the actual values of numbers in NatSpec comments makes checking and reading code easier

File: contracts/Position.sol
- 53:         require(initPeriod >= 3 days); // must be at least three days, recommended to use higher values
+ 53:         require(initPeriod >= 3 days); // must be at least three days, recommended to use higher values, 3 Days:  259200 (3 * 24 * 60 * 60 )
- 161:            restrictMinting(3 days);
+ 161:            restrictMinting(3 days); // 3 Days:  259200 (3 * 24 * 60 * 60 )
- 312:            restrictMinting(1 days);
+ 312:            restrictMinting(1 days); // 1 Days:  86400 (1 * 24 * 60 * 60 )

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L53 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L161 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L312

File: contracts/MintingHub.sol
- 64:            7 days, _expirationSeconds, _challengeSeconds, _mintingFeePPM, _liqPrice, _reservePPM);
+ 64:            7 days, _expirationSeconds, _challengeSeconds, _mintingFeePPM, _liqPrice, _reservePPM); // 7 Days:  604800 (7 * 24 * 60 * 60 )

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L64

Gas Optimizations

Issue

[G-01] Use nested if and, avoid multiple check combinations ( 04 instances )

[G-02] Upgrade Solidity’s optimizer ( 01 instances )

[G-03] Avoid using state variable in emit ( 05 instances )

[G-04] Failure to check the zero address in the constructor causes the contract to be deployed again ( 03 instances )

[G-05] Unchecked arithmetic ( 03 instances )

[G-06] Use of named returns for local variables saves gas ( 04 instances )

[G-07] >=/<= costs less gas than >/< ( 04 instances )

Total: 28 instances over 07 issues

[G-01] Use nested if and, avoid multiple check combinations ( 04 instances )

Description:

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

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();
267:      if (!isMinter(msg.sender) && !isMinter(positions[msg.sender])) revert NotMinter();

Frankencoin.sol#84-85 Frankencoin#267

[G-02] Upgrade Solidity’s optimizer

Description:

Make sure Solidity’s optimizer is enabled. It reduces gas costs. If you want to gas optimize for contract deployment (costs less to deploy a contract) then set the Solidity optimizer at a low number. If you want to optimize for run-time gas costs (when functions are called on a contract) then set the optimizer to a high number. Set the optimization value higher than 800 in your hardhat.config.js file.

96:          optimizer: {
97:              enabled: true,
98:              runs: 200
99:              },

Link to code Please visit the following site for further information: https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

[G-03] Avoid using state variable in emit ( 05 instances )

File: contracts/Position.sol
\\ @audit original 
69:        emit PositionOpened(_owner, original, _zchf, address(collateral), _liqPrice);

Link to code

File: contracts/MintingHub.sol
\\ @audit pos
146:        emit ChallengeStarted(msg.sender, address(position), _collateralAmount, pos);
177:        emit ChallengeStarted(copy.challenger, address(copy.position), copy.size, pos);
\\ @audit collateral
292:            emit PostPonedReturn(collateral, challenge.challenger, challenge.size);

Link to code 146 Link to code 177 Link to code 292

File: contracts/Equity.sol
\\ @audit proceeds
280:        emit Trade(msg.sender, -int(shares), proceeds, price());

Link to code

[G-04] Failure to check the zero address in the constructor causes the contract to be deployed again ( 03 instances )

Zero address control is not performed in the constructor in all 3 contracts within the scope of the audit. Bypassing this check could cause the contract to be deployed by mistakenly entering a zero address. In this case, the contract will need to be redeployed. This means extra gas consumption as contract deployment costs are high.

    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);
    }

contracts/Position.sol

    constructor(address _zchf, address factory) {
        zchf = IFrankencoin(_zchf);
        POSITION_FACTORY = IPositionFactory(factory);
    }

contracts/MintingHub.sol

    constructor(address other, address zchfAddress, uint256 limit_){
        chf = IERC20(other);
        zchf = IFrankencoin(zchfAddress);
        horizon = block.timestamp + 52 weeks;
        limit = limit_;
    }

contracts/StablecoinBridge.sol

[G-05] Unchecked arithmetic ( 03 instances )

The default “checked” behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected. If it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas.

File: contracts/Position.sol
203:        uint256 horizon = block.timestamp + period;

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L203

File: contracts/MintingHub.sol
217:            uint256 earliestEnd = block.timestamp + 30 minutes;

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L217

File: contracts/Frankencoin.sol
88:      minters[_minter] = block.timestamp + _applicationPeriod;

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L88

Recommendation:

Place the arithmetic operations in an unchecked block

[G-06] Use of named returns for local variables saves gas ( 04 instances )

You can have further advantages in terms of gas cost by simply using named return values as temporary local variable.

For instance, the code block below may be refactored as follows:

File: contracts/Position.sol
- 304:     function tryAvertChallenge(uint256 _collateralAmount, uint256 _bidAmountZCHF) external onlyHub returns (bool) {
+ 304:     function tryAvertChallenge(uint256 _collateralAmount, uint256 _bidAmountZCHF) external onlyHub returns (bool _status) {

             [ ... ]
- 315:            return false;
+ 315:           status_ = false;
316:        }
317:    }

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L304-L317

File: contracts/Equity.sol
225:    function canVoteFor(address delegate, address owner) internal view returns (bool) {
241:    function onTokenTransfer(address from, uint256 amount, bytes calldata) external returns (bool) {

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L225-L233 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L241-L255

File: contracts/StablecoinBridge.sol
75:    function onTokenTransfer(address from, uint256 amount, bytes calldata) external returns (bool){

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/StablecoinBridge.sol#L75-L84

[G-07] >=/<= costs less gas than >/< ( 04 instances )

The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas

File: contracts/Position.sol
349:        uint256 repayment = minted < volumeZCHF ? minted : volumeZCHF; // how much must be burned to make things even

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L349

File: contracts/Equity.sol
268:        uint256 newTotalShares = totalShares < 1000 * ONE_DEC18 ? 1000 * ONE_DEC18 : _mulD18(totalShares, _cubicRoot(_divD18(capitalBefore + investment, capitalBefore)));

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L268

File: contracts/Frankencoin.sol
238:      uint256 adjustedReservePPM = currentReserve < minterReserve_ ? reservePPM * currentReserve / minterReserve_ : reservePPM; // 18%

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L238

File: contracts/MathUtil.sol
26:            cond = xOld > x ? xOld - x > THRESH_DEC18 : x - xOld > THRESH_DEC18;

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MathUtil.sol#L26