Frankencoin - Sathish9098's results

LOW FINDINGS

[L-1] Loss of precision due to rounding

FILE: 2023-04-frankencoin/contracts/Position.sol

80:  price = _mint * ONE_DEC18 / _coll;
98:  uint256 reduction = (limit - minted - _minimum)/2; 
122: return totalMint * (1000_000 - reserveContribution - calculateCurrentFee()) / 1000_000;
124: return totalMint * (1000_000 - reserveContribution) / 1000_000;
187: return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));
239: return 1000000 * amountExcludingReserve / (1000000 - adjustedReservePPM); // 41 / (1-18%) = 50

Position.sol#L80

FILE: FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

118: return minterReserveE6 / 1000000;

Frankencoin.sol#L118

FILE: 2023-04-frankencoin/contracts/Equity.sol

109:  return VALUATION_FACTOR * zchf.equity() * ONE_DEC18 / totalSupply();
161:  voteAnchor[to] = uint64(anchorTime() - recipientVotes / newbalance); 
173:  return uint64(block.number << BLOCK_TIME_RESOLUTION_BITS);

Equity.sol#L109

FILE: 2023-04-frankencoin/contracts/MintingHub.sol

189:   return (challenge.bid * 1005) / 1000;

MintingHub.sol#L189

[L-2] Consider using OpenZeppelin’s SafeCast library to prevent unexpected overflows when casting from uint256

Using the SafeCast library can help prevent unexpected errors in your Solidity code and make your contracts more secure

FILE: 2023-04-frankencoin/contracts/Position.sol

187: return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));

Position.sol#L187

FILE: 2023-04-frankencoin/contracts/Equity.sol

146:   totalVotesAtAnchor = uint192(totalVotes() - roundingLoss - lostVotes);
161:   voteAnchor[to] = uint64(anchorTime() - recipientVotes / newbalance); 

Equity.sol#L146

Recommended Mitigation Steps:

Consider using OpenZeppelin’s SafeCast library to prevent unexpected overflows when casting from uint256.

[L-3] Vulnerable to cross-chain replay attacks due to static DOMAIN_SEPARATOR/domainSeparator

See this issue from a prior contest for details

FILE: 2023-04-frankencoin/contracts/ERC20PermitLight.sol

function DOMAIN_SEPARATOR() public view returns (bytes32) {
        return
            keccak256(
                abi.encode(
                    //keccak256("EIP712Domain(uint256 chainId,address verifyingContract)");
                    bytes32(0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218),
                    block.chainid,
                    address(this)
                )
            );

ERC20PermitLight.sol#L61-L70

[L-4] MIXING AND OUTDATED COMPILER

The pragma version used are: 0.8.0

The minimum required version must be 0.8.17; otherwise, contracts will be affected by the following important bug fixes:

0.8.14:

ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases. Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays. Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

0.8.17 Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call. Apart from these, there are several minor bug fixes and improvements

FILE: 2023-04-frankencoin/contracts/ERC20.sol

12: pragma solidity ^0.8.0;

FILE: 2023-04-frankencoin/contracts/ERC20PermitLight.sol

5: pragma solidity ^0.8.0;

FILE: 2023-04-frankencoin/contracts/StablecoinBridge.sol

2: pragma solidity ^0.8.0;

FILE: 2023-04-frankencoin/contracts/StablecoinBridge.sol

2: pragma solidity ^0.8.0;

FILE: 2023-04-frankencoin/contracts/PositionFactory.sol

2: pragma solidity ^0.8.0;

FILE: 2023-04-frankencoin/contracts/MathUtil.sol

3: pragma solidity >=0.8.0 <0.9.0;

FILE: 2023-04-frankencoin/contracts/Ownable.sol

9: pragma solidity ^0.8.0;

FILE: 2023-04-frankencoin/contracts/Position.sol

2: pragma solidity ^0.8.0;

FILE: 2023-04-frankencoin/contracts/MintingHub.sol

2: pragma solidity ^0.8.0;

FILE: 2023-04-frankencoin/contracts/Equity.sol

4: pragma solidity >=0.8.0 <0.9.0;

FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

2: pragma solidity ^0.8.0;

[L-5] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead. If all arguments are strings and or bytes, bytes.concat() should be used instead

FILE: 2023-04-frankencoin/contracts/ERC20PermitLight.sol

abi.encodePacked(
                        "\x19\x01",
                        DOMAIN_SEPARATOR(),
                        keccak256(
                            abi.encode(
                                // keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"),
                                bytes32(0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9),
                                owner,
                                spender,
                                value,
                                nonces[owner]++,
                                deadline
                            )
                        )
                    )
                ),
                v,
                r,
                s
            );

ERC20PermitLight.sol#L35-L54

[L-6] Lack of Sanity/Threshold/Limit Checks

Devoid of sanity/threshold/limit checks, critical parameters can be configured to invalid values, causing a variety of issues and breaking expected interactions within/between contracts. Consider adding proper uint256 validation as well as zero address checks for critical changes. A worst case scenario would render the contract needing to be re-deployed in the event of human/accidental errors that involve value assignments to immutable variables. If the validation procedure is unclear or too complex to implement on-chain, document the potential issues that could produce invalid values

FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

_minApplicationPeriod value is not checked before assigning to MIN_APPLICATION_PERIOD 

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

function registerPosition(address _position) override external {
      if (!isMinter(msg.sender)) revert NotMinter();
      positions[_position] = msg.sender;
   }

function burn(uint256 _amount) external {
      _burn(msg.sender, _amount);
   }

function isMinter(address _minter) override public view returns (bool){
      return minters[_minter] != 0 && block.timestamp >= minters[_minter];
   }

Frankencoin.sol#L59-L62

FILE: 2023-04-frankencoin/contracts/Equity.sol

112: function _beforeTokenTransfer(address from, address to, uint256 amount) override internal {
113: super._beforeTokenTransfer(from, to, amount);

Equity.sol#L112-L113

FILE: 2023-04-frankencoin/contracts/MintingHub.sol

function openPosition(
        address _collateralAddress, uint256 _minCollateral, uint256 _initialCollateral,
        uint256 _mintingMaximum, uint256 _initPeriodSeconds, uint256 _expirationSeconds, uint256 _challengeSeconds,
        uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) public returns (address) {

MintingHub.sol#L88-L105

[L-7] Function Calls in Loop Could Lead to Denial of Service

Function calls made in unbounded loop are error-prone with potential resource exhaustion as it can trap the contract due to the gas limitations or failed transactions

FILE: 2023-04-frankencoin/contracts/Equity.sol

for (uint i=0; i<helpers.length; i++){
            address current = helpers[i];

 for (uint256 i = 0; i<addressesToWipe.length; i++){
            address current = addressesToWipe[0];

Equity.sol#L192

[L-8] Use .call instead of .transfer to send ether

.transfer will relay 2300 gas and .call will relay all the gas. If the receive/fallback function from the recipient proxy contract has complex logic, using .transfer will fail, causing integration issues

FILE: 2023-04-frankencoin/contracts/Equity.sol

279: zchf.transfer(target, proceeds);

Equity.sol#L279

FILE: 2023-04-frankencoin/contracts/MintingHub.sol

294:  challenge.position.collateral().transfer(challenge.challenger, challenge.size);
204:  zchf.transfer(challenge.bidder, challenge.bid); // return old bid
211:  challenge.position.collateral().transfer(msg.sender, challenge.size);
268:  zchf.transfer(owner, effectiveBid - fundsNeeded);

MintingHub.sol#L294

FILE: 2023-04-frankencoin/contracts/Position.sol

253: IERC20(token).transfer(target, amount);
269: IERC20(collateral).transfer(target, amount);

Position.sol#L253

[L-9] 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

[L-10] ALLOWS MALLEABLE SECP256K1 SIGNATURES

Here, the ecrecover() method doesn’t check the s range.

Homestead (EIP-2) added this limitation, however the precompile remained unaltered. The majority of libraries, including OpenZeppelin, do this check.

Since an order can only be confirmed once and its hash is saved, there doesn’t seem to be a serious danger in existing use cases

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/7201e6707f6631d9499a569f492870ebdd4133cf/contracts/utils/cryptography/ECDSA.sol#L138-L149

FILE: 2023-04-frankencoin/contracts/ERC20PermitLight.sol

 address recoveredAddress = ecrecover(
                keccak256(
                    abi.encodePacked(
                        "\x19\x01",
                        DOMAIN_SEPARATOR(),
                        keccak256(
                            abi.encode(
                                // keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"),
                                bytes32(0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9),
                                owner,
                                spender,
                                value,
                                nonces[owner]++,
                                deadline
                            )
                        )
                    )
                ),
                v,
                r,
                s
            );

ERC20PermitLight.sol#L33-L54

[L-11] AVOID HARDCODED VALUES

It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these values from the source code

FILE: 2023-04-frankencoin/contracts/ERC20PermitLight.sol

41:  bytes32(0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9),
66:  bytes32(0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218),

ERC20PermitLight.sol#L41

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

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

[L-12] Front running attacks by the onlyOwner

owner value is not a constant value and can be changed with transferOwnership() function, before a function using setOwner state variable value in the project, transferOwnership function can be triggered by onlyOwner and operations can be blocked

FILE: 2023-04-frankencoin/contracts/Ownable.sol

 function transferOwnership(address newOwner) public onlyOwner {
        setOwner(newOwner);
    }

    /**
     * @dev Transfers ownership of the contract to a new account (`newOwner`).
     * Internal function without access restriction.
     */
    function setOwner(address newOwner) internal {
        address oldOwner = owner;
        owner = newOwner;
        emit OwnershipTransferred(oldOwner, newOwner);
    }

NON CRITICAL FINDINGS

[NC-1] immutable should be uppercase

FILE: 2023-04-frankencoin/contracts/Position.sol
uint256 public immutable start; // timestamp when minting can start
uint256 public immutable expiration; // timestamp at which the position expires
address public immutable original; // originals point to themselves, clone to their origin
address public immutable hub; // the hub this position was created by
IFrankencoin public immutable zchf; // currency
IERC20 public override immutable collateral; // collateral
uint256 public override immutable minimumCollateral; // prevent dust amounts
uint32 public immutable mintingFeePPM;
uint32 public immutable reserveContribution; // in ppm

Position.sol#L29-L39

FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

31: IReserve override public immutable reserve;

Frankencoin.sol#L31

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

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

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

[NC-2] Missing NATSPEC

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

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

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

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

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

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

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

[NC-3] For functions, follow Solidity standard naming conventions (internal function style rule)

Description

The above codes don’t follow Solidity’s standard naming convention,

internal and private functions : the mixedCase format starting with an underscore (_mixedCase starting with an underscore)

FILE: 2023-04-frankencoin/contracts/Position.sol

193: function mintInternal(address target, uint256 amount, uint256 collateral_) internal {
202: function restrictMinting(uint256 period) internal {
232: function repayInternal(uint256 burnable) internal {
240: function notifyRepaidInternal(uint256 amount) internal 
268: function internalWithdrawCollateral(address target, uint256 amount) internal returns (uint256) {
282: function checkCollateral(uint256 collateralReserve, uint256 atPrice) internal view {
286: function emitUpdate() internal {

Position.sol#L193

FILE: FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

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

Frankencoin.sol#L102

FILE: 2023-04-frankencoin/contracts/Equity.sol

144:  function adjustTotalVotes(address from, uint256 amount, uint256 roundingLoss) internal {
157:  function adjustRecipientVoteAnchor(address to, uint256 amount) internal returns (uint256){

Equity.sol#L144

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

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

[NC-4] 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

FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

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

Frankencoin.sol#L25

FILE: 2023-04-frankencoin/contracts/MintingHub.sol

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

MintingHub.sol#L20

[NC-5] Need Fuzzing test for unchecked

FILE: 2023-04-frankencoin/contracts/ERC20PermitLight.sol

32: unchecked { 

ERC20PermitLight.sol#L32

[NC-6] Remove commented out code

FILE: 2023-04-frankencoin/contracts/ERC20PermitLight.sol

65: //keccak256("EIP712Domain(uint256 chainId,address verifyingContract)");

ERC20PermitLight.sol#L65

[NC-7] Inconsistent spacing in comments

Some lines use // x and some use //x. The instances below point out the usages that don’t follow the majority, within each file

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

[NC-8] Inconsistent method of specifying a floating pragma

Some files use >=, some use ^. The instances below are examples of the method that has the fewest instances for a specific version. Note that using >= without also specifying <= will lead to failures to compile, or external project incompatability, when the major version changes and there are breaking-changes, so ^ should be preferred regardless of the instance counts

FILE: 2023-04-frankencoin/contracts/PositionFactory.sol

2: pragma solidity ^0.8.0;

FILE: 2023-04-frankencoin/contracts/MathUtil.sol

3: pragma solidity >=0.8.0 <0.9.0;

[NC-9] Numeric values having to do with time should use time units for readability

There are units for seconds, minutes, hours, days, and weeks, and since they’re defined, they should be used

FILE: 2023-04-frankencoin/contracts/Equity.sol

51:  uint8 private constant BLOCK_TIME_RESOLUTION_BITS = 24;

59:  uint256 public constant MIN_HOLDING_DURATION = 90*7200 << BLOCK_TIME_RESOLUTION_BITS;

Equity.sol#L59

[NC-10] NO SAME VALUE INPUT CONTROL

FILE : 2023-04-frankencoin/contracts/Ownable.sol

  function setOwner(address newOwner) internal {
        address oldOwner = owner;
        owner = newOwner;
        emit OwnershipTransferred(oldOwner, newOwner);
    }

Ownable.sol#L39-L43

[NC-11] Contract layout and order of functions

The Solidity style guide recommends

Declare internal functions bellow the external/public functions

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

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

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

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

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/ERC20.sol#L97-L108

[NC-12] Constant redefined elsewhere

Consider defining in only one contract so that values cannot become out of sync when only one location is updated.

A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract’s value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don’t get out of sync.

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

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

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

[NC-13] According to the syntax rules, use => mapping ( instead of => mapping( using spaces as keyword

FILE: 2023-04-frankencoin/contracts/ERC20PermitLight.sol

- 15: mapping(address => uint256) public nonces;
+ 15: mapping (address => uint256) public nonces;

ERC20PermitLight.sol#L15

[NC-14] Tokens accidentally sent to the contract cannot be recovered

It can’t be recovered if the tokens accidentally arrive at the contract address, which has happened to many popular projects, so I recommend adding a recovery code to your critical contracts

Recommended Mitigation Steps

Add this code:

 /**
  * @notice Sends ERC20 tokens trapped in contract to external address
  * @dev Onlyowner is allowed to make this function call
  * @param account is the receiving address
  * @param externalToken is the token being sent
  * @param amount is the quantity being sent
  * @return boolean value indicating whether the operation succeeded.
  *
 */
  function rescueERC20(address account, address externalToken, uint256 amount) public onlyOwner returns (bool) {
    IERC20(externalToken).transfer(account, amount);
    return true;
  }
}

[NC-15] 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

[NC-16] Constants on the left are better

If you use the constant first you support structures that veil programming errors. And one should only produce code either to add functionality, fix an programming error or trying to support structures to avoid programming errors (like design patterns).

https://www.moserware.com/2008/01/constants-on-left-are-better-but-this.html

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

[NC-17] Assembly Codes Specific – Should Have Comments

Since this is a low level language that is more difficult to parse by readers, include extensive documentation, comments on the rationale behind its use, clearly explaining what each assembly instruction does.

This will make it easier for users to trust the code, for reviewers to validate the code, and for developers to build on or update the code.

Note that using Assembly removes several important security features of Solidity, which can make the code more insecure and more error-prone

FILE: 2023-04-frankencoin/contracts/PositionFactory.sol

39:   assembly {

PositionFactory.sol#L39

[NC-18] Take advantage of Custom Error’s return value property

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: 2023-04-frankencoin/contracts/Position.sol

77:  if(_coll < minimumCollateral) revert InsufficientCollateral();
110: if (block.timestamp >= start) revert TooLate();
194: if (minted + amount > limit) revert LimitExceeded();
294: if (size < minimumCollateral && size < collateralBalance()) revert ChallengeTooSmall();

Position.sol#L77

[NC-19] Use constants instead of using numbers directly without explanations

FILE: 2023-04-frankencoin/contracts/Position.sol

122: return totalMint * (1000_000 - reserveContribution - calculateCurrentFee()) / 1000_000;
124: return totalMint * (1000_000 - reserveContribution) / 1000_000;

Position.sol#L122

FILE: 2023-04-frankencoin/contracts/MintingHub.sol

189: return (challenge.bid * 1005) / 1000;
217: uint256 earliestEnd = block.timestamp + 30 minutes;
265: uint256 reward = (volume * CHALLENGER_REWARD) / 1000_000;

MintingHub.sol#L189

FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

118: return minterReserveE6 / 1000000;
166: uint256 usableMint = (_amount * (1000_000 - _feesPPM - _reservePPM)) / 1000_000; // rounding down is fine
205: uint256 theoreticalReserve = _reservePPM * mintedAmount / 1000000;
239: return 1000000 * amountExcludingReserve / (1000000 - adjustedReservePPM); // 41 / (1-18%) = 50

Frankencoin.sol#L118

[NC-20] Shorthand way to write if / else statement

The normal if / else statement can be refactored in a shorthand way to write it:

Increases readability Shortens the overall SLOC

FILE: 2023-04-frankencoin/contracts/Position.sol

184: if (time >= exp){
            return 0;
        } else {
            return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));
        }

160: if (newPrice > price) {
            restrictMinting(3 days);
        } else {
            checkCollateral(collateralBalance(), newPrice);
        }

121: if (afterFees){
            return totalMint * (1000_000 - reserveContribution - calculateCurrentFee()) / 1000_000;
        } else {
            return totalMint * (1000_000 - reserveContribution) / 1000_000;
        }

250: if (token == address(collateral)){
            withdrawCollateral(target, amount);
        } else {
            IERC20(token).transfer(target, amount);
        }

Position.sol#L184-L188

FILE: 2023-04-frankencoin/contracts/MintingHub.sol

267: if (effectiveBid > fundsNeeded){
            zchf.transfer(owner, effectiveBid - fundsNeeded);
        } else if (effectiveBid < fundsNeeded){
            zchf.notifyLoss(fundsNeeded - effectiveBid); // ensure we have enough to pay everything
        }

MintingHub.sol#L267-L271

FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

141: if (balance <= minReserve){
        return 0;
      } else {
        return balance - minReserve;
      }

207: if (currentReserve < minterReserve()){
         // not enough reserves, owner has to take a loss
         return theoreticalReserve * currentReserve / minterReserve();
      } else {
         return theoreticalReserve;
      }

Frankencoin.sol#L141-L145

Recommended Mitigation

time >= exp ? return 0 : return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));

[NC-21] Return values of approve() not checked

Not all IERC20 implementations revert() when there’s a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything

FILE: 2023-04-frankencoin/contracts/ERC20.sol

109: _approve(msg.sender, spender, value);
132: _approve(sender, msg.sender, currentAllowance - amount);

ERC20.sol#L109

GAS OPTIMIZATION

[G-1] State variables should be cached in stack variables rather than re-reading them from storage

Instances(2)

Approximate gas saved: 500 gas

Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read. Less obvious fixes/optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

FILE: 2023-04-frankencoin/contracts/Position.sol

minted state variable should be cached with stack variable 

141: if (newMinted < minted){ /// @audit minted cached
            zchf.burnFrom(msg.sender, minted - newMinted, reserveContribution); /// @audit minted cached
            minted = newMinted;
        }
        if (newCollateral < colbal){
            withdrawCollateral(msg.sender, colbal - newCollateral);
        }
        // Must be called after collateral withdrawal
        if (newMinted > minted){  /// @audit minted cached
            mint(msg.sender, newMinted - minted);  /// @audit minted cached
        }

241: if (amount > minted) revert RepaidTooMuch(amount - minted);
        minted -= amount;

349:  uint256 repayment = minted < volumeZCHF ? minted : volumeZCHF; 

Position.sol#L141-L150

[G-2] Using storage instead of memory for structs/arrays saves gas

Instances(2]1)

Approximate gas saved: 2100 gas

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct

FILE: 2023-04-frankencoin/contracts/MintingHub.sol

159: Challenge memory copy = Challenge(

MintingHub.sol#L159

[G-3] For events use 3 indexed rule to save gas

Instances(12)

Need to declare 3 indexed fields for event parameters. If the event parameter is less than 3 should declare all event parameters indexed

FILE: 2023-04-frankencoin/contracts/Position.sol

41: event PositionOpened(address indexed owner, address original, address zchf, address collateral, uint256 price);
42: event MintingUpdate(uint256 collateral, uint256 price, uint256 minted, uint256 limit);
43: event PositionDenied(address indexed sender, string message); // emitted if closed by governance

Position.sol#L41-L43

FILE: 2023-04-frankencoin/contracts/MintingHub.sol

48: event ChallengeStarted(address indexed challenger, address indexed position, uint256 size, uint256 number);
49: event ChallengeAverted(address indexed position, uint256 number);
50: event ChallengeSucceeded(address indexed position, uint256 bid, uint256 number);
51: event NewBid(uint256 challengedId, uint256 bidAmount, address bidder);
52: event PostPonedReturn(address collateral, address indexed beneficiary, uint256 amount);

MintingHub.sol#L48-L52

FILE: 2023-04-frankencoin/contracts/Equity.sol

90: event Delegation(address indexed from, address indexed to); // indicates a delegation
91: event Trade(address who, int amount, uint totPrice, uint newprice); // amount pos or neg for mint or redemption

Equity.sol#L90-L91

FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

52: event MinterApplied(address indexed minter, uint256 applicationPeriod, uint256 applicationFee, string message);
53: event MinterDenied(address indexed minter, string message);

Frankencoin.sol#L52-L53

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

Instances(6)

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. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations.

FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

45: mapping (address => uint256) public minters;
50: mapping (address => address) public positions;

Frankencoin.sol#L45-L50

FILE: 2023-04-frankencoin/contracts/Equity.sol

83: mapping (address => address) public delegates;
88: mapping (address => uint64) private voteAnchor;

Equity.sol#L83-L88

FILE: 2023-04-frankencoin/contracts/ERC20.sol

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

ERC20.sol#L43-L45

[G-5] Lack of input value checks cause a redeployment if any human/accidental errors

Instances(24)

Devoid of sanity/threshold/limit checks, critical parameters can be configured to invalid values, causing a variety of issues and breaking expected interactions within/between contracts. Consider adding proper uint256 validation. A worst case scenario would render the contract needing to be re-deployed in the event of human/accidental errors that involve value assignments to immutable variables.

If any human/accidental errors happen need to redeploy the contract so this create the huge gas lose

FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

_minApplicationPeriod value is not checked before assigning to MIN_APPLICATION_PERIOD 

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

Frankencoin.sol#L59-L62

FILE: 2023-04-frankencoin/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);
    }

Position.sol#L50-L70

FILE: 2023-04-frankencoin/contracts/MintingHub.sol

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

MintingHub.sol#L54-L57

FILE: 2023-04-frankencoin/contracts/Equity.sol

 constructor(Frankencoin zchf_) ERC20(18) {
        zchf = zchf_;
    }

Equity.sol#L93-L95

FILE: 2023-04-frankencoin/contracts/StablecoinBridge.sol

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

StablecoinBridge.sol#L26-L31

[G-6] Use nested if and, avoid multiple check combinations

Instances(4)

Approximate Gas Saved: 36 gas

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

As per Solidity reports possible to save 9 gas

FILE: FILE: 2023-04-frankencoin/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#L84-L85

FILE: 2023-04-frankencoin/contracts/Position.sol

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

Position.sol#L294

[G-7] No need to evaluate all expressions to know if one of them is true

Instances(1)

When we have a code expressionA || expressionB if expressionA is true then expressionB will not be evaluated and gas saved

FILE: FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

106: } else if (isMinter(spender) || isMinter(isPosition(spender))){

Frankencoin.sol#L106

[G-8] The result of function calls should be cached rather than re-calling the function

Instances(2)

In Solidity, caching repeated function calls can be an effective way to optimize gas usage, especially when the function is called frequently with the same arguments

FILE: FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

totalSupply() value should be cached instead of calling multiple times 

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

Frankencoin.sol#L84-L85

FILE: 2023-04-frankencoin/contracts/Equity.sol

anchorTime() function results should be cached 

function adjustTotalVotes(address from, uint256 amount, uint256 roundingLoss) internal {
        uint256 lostVotes = from == address(0x0) ? 0 : (anchorTime() - voteAnchor[from]) * amount;
        totalVotesAtAnchor = uint192(totalVotes() - roundingLoss - lostVotes);
        totalVotesAnchorTime = anchorTime();
    }

Equity.sol#L144-L148

[G-9] Amounts should be checked for 0 before calling a transfer

Instances(10)

Checking non-zero transfer values can avoid an expensive external call and save gas. While this is done at some places, it’s not consistently done in the solution. I suggest adding a non-zero-value check here

FILE: FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

87:  _transfer(msg.sender, address(reserve), _applicationFee);
283: _transfer(address(reserve), msg.sender, _amount);
285: _transfer(address(reserve), msg.sender, reserveLeft);
254: _transfer(address(reserve), msg.sender, freedAmount - _amountExcludingReserve); 

Frankencoin.sol#L87

FILE: 2023-04-frankencoin/contracts/Equity.sol

279:    zchf.transfer(target, proceeds);

Equity.sol#L279

FILE: 2023-04-frankencoin/contracts/MintingHub.sol

108:  zchf.transferFrom(msg.sender, address(zchf.reserve()), OPENING_FEE);
129:  existing.collateral().transferFrom(msg.sender, address(pos), _initialCollateral);
225:  zchf.transferFrom(msg.sender, address(this), _bidAmountZCHF);

MintingHub.sol#L108

[G-10] Don't declare the variable inside the loops

Instances(2)

In every iterations the new variables instance created this will consumes more gas . So just declare variables outside the loop and only use inside to save gas

FILE: 2023-04-frankencoin/contracts/Equity.sol

192: for (uint i=0; i<helpers.length; i++){
            address current = helpers[i];

312: for (uint256 i = 0; i<addressesToWipe.length; i++){
            address current = addressesToWipe[0];

Equity.sol#L192-L193

[G-11] Empty blocks should be removed to save deployment cost

Instances(1)

FILE: ERC20.sol

240: function _beforeTokenTransfer(address from, address to, uint256 amount) virtual internal {
    }

ERC20.sol#L240

[G-12] Functions should be used instead of modifiers to save gas

Instances(3)

FILE: 2023-04-frankencoin/contracts/Position.sol

modifier noCooldown() {
        if (block.timestamp <= cooldown) revert Hot();
        _;
    }


    modifier noChallenge() {
        if (challengedAmount > 0) revert Challenged();
        _;
    }


    modifier onlyHub() {
        if (msg.sender != address(hub)) revert NotHub();
        _;
    }

Position.sol#L373-L390

[G-13] Avoid contract existence checks by using low level calls

Instances(5)

Approximate gas saved: 500 gas

Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external function calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value. Similar behavior can be achieved in earlier versions by using low-level calls, since low level calls never check for contract existence

FILE: 2023-04-frankencoin/contracts/Equity.sol

109:  return VALUATION_FACTOR * zchf.equity() * ONE_DEC18 / totalSupply();
243:  uint256 equity = zchf.equity();
279:  zchf.transfer(target, proceeds);
292:  uint256 capital = zchf.equity();
310:  require(zchf.equity() < MINIMUM_EQUITY); 

Equity.sol#L109

[G-14] Sort Solidity operations using short-circuit mode

Instances(3)

Short-circuiting is a solidity contract development model that uses OR/AND logic to sequence different cost operations. It puts low gas cost operations in the front and high gas cost operations in the back, so that if the front is low If the cost operation is feasible, you can skip (short-circuit) the subsequent high-cost Ethereum virtual machine operation.

//f(x) is a low gas cost operation 
//g(y) is a high gas cost operation 
//Sort operations with different gas costs as follows 
f(x) || g(y) 
f(x) && g(y)

FILE: 2023-04-frankencoin/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#L84-L85

[G-15] Use assembly to check for address(0)

Instances(3)

Saves 6 gas per instance

FILE: 2023-04-frankencoin/contracts/ERC20.sol

152: require(recipient != address(0));
180: require(recipient != address(0));

ERC20.sol#L152

FILE: 2023-04-frankencoin/contracts/ERC20PermitLight.sol

56: require(recoveredAddress != address(0) && recoveredAddress == owner, "INVALID_SIGNER");

ERC20PermitLight.sol#L56

[G-16] Shorthand way to write if / else statement can reduce the deployment cost

Instances(7)

The normal if / else statement can be refactored in a shorthand way to write it:

Increases readability Shortens the overall SLOC

FILE: 2023-04-frankencoin/contracts/Position.sol

184: if (time >= exp){
            return 0;
        } else {
            return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));
        }

160: if (newPrice > price) {
            restrictMinting(3 days);
        } else {
            checkCollateral(collateralBalance(), newPrice);
        }

121: if (afterFees){
            return totalMint * (1000_000 - reserveContribution - calculateCurrentFee()) / 1000_000;
        } else {
            return totalMint * (1000_000 - reserveContribution) / 1000_000;
        }

250: if (token == address(collateral)){
            withdrawCollateral(target, amount);
        } else {
            IERC20(token).transfer(target, amount);
        }

Position.sol#L184-L188

FILE: 2023-04-frankencoin/contracts/MintingHub.sol

267: if (effectiveBid > fundsNeeded){
            zchf.transfer(owner, effectiveBid - fundsNeeded);
        } else if (effectiveBid < fundsNeeded){
            zchf.notifyLoss(fundsNeeded - effectiveBid); // ensure we have enough to pay everything
        }

MintingHub.sol#L267-L271

FILE: 2023-04-frankencoin/contracts/Frankencoin.sol

141: if (balance <= minReserve){
        return 0;
      } else {
        return balance - minReserve;
      }

207: if (currentReserve < minterReserve()){
         // not enough reserves, owner has to take a loss
         return theoreticalReserve * currentReserve / minterReserve();
      } else {
         return theoreticalReserve;
      }

Frankencoin.sol#L141-L145

Recommended Mitigation

time >= exp ? return 0 : return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));

[G-17] The Less gas consuming condition checks should be on top

Instances(1)

When writing conditional statements in smart contracts, it is generally best practice to order the conditions so that the less gas-consuming checks are performed first. This can help to optimize the gas usage of the contract and improve its overall efficiency

FILE: 2023-04-frankencoin/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();

Frankencoin.sol#L84-L86

[G-18] internal functions not called by the contract should be removed to save deployment gas

Instances(4)

If the functions are required by an interface, the contract should inherit from that interface and use the override keyword

FILE: 2023-04-frankencoin/contracts/MathUtil.sol

18: function _cubicRoot(uint256 _v) internal pure returns (uint256) {
39: function _power3(uint256 _x) internal pure returns(uint256) {

MathUtil.sol#L18

FILE: 2023-04-frankencoin/contracts/ERC20.sol

179: function _mint(address recipient, uint256 amount) internal virtual {
200: function _burn(address account, uint256 amount) internal virtual {

ERC20.sol#L179

[G-19] Using bools for memory incurs overhead

Instances(1)

Use uint256(1) and uint256(2) for true/false

FILE: 2023-04-frankencoin/contracts/MathUtil.sol

21:   bool cond;

MathUtil.sol#L21

[G-20] Avoid emitting constants

Instances(2)

One way to optimize your smart contract and reduce the gas cost is to avoid emitting constants. When you declare a constant in your contract, it is stored on the blockchain and takes up space. This can increase the cost of deploying your contract and make it more expensive to execute.

To avoid emitting constants, you can use inline assembly to perform arithmetic operations or bitwise operations. You can also use local variables instead of constants, and calculate the values you need at runtime

FILE: 2023-04-frankencoin/contracts/Position.sol

original,collateral are immutable constants 

69: emit PositionOpened(_owner, original, _zchf, address(collateral), _liqPrice);
85: emit PositionOpened(owner, original, address(zchf), address(collateral), _price);

Position.sol#L69