Frankencoin - RaymondFam's results

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L97-L101 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L124-L132

Vulnerability details

Impact

A new position costing 1000 ZCHF could have the entire limit taken up by users calling clonePosition(). As a result, the position ends up being a free service provider to others.

Proof of Concept

Here is a typical scenario:

1. Alice opens a new position with 10_000 ZCHF minting limit and is putting off the idea of minting any ZCHF for the sake of saving a little bit on mintingFeePPM.
2. Bob, upon seeing the position cooldown has elapsed, proceeds to calling MintingHub.clonePosition() with _initialMint that equals 10_000 ZCHF. reduceLimitForClone() is first invoked where:

reduction = (10_000 - 0 - 10_000)/2 = 0/2 = 0 limit = 10_000 - (0 + 10_000) = 0 return = 0 + 10_000 = 10_000

File: Position.sol#L97-L101

    function reduceLimitForClone(uint256 _minimum) external noChallenge noCooldown alive onlyHub returns (uint256) {
        uint256 reduction = (limit - minted - _minimum)/2; // this will fail with an underflow if minimum is too high
        limit -= reduction + _minimum;
        return reduction + _minimum;
    }

3. The returned value of 10_000 is assigned to limit on line 126 of the code block below. After having the clone position registered and the needed collateral transferred to the cloned Position contract, initializeClone() is externally called:

File: MintingHub.sol#L124-L132

    function clonePosition(address position, uint256 _initialCollateral, uint256 _initialMint) public validPos(position) returns (address) {
        IPosition existing = IPosition(position);
126:        uint256 limit = existing.reduceLimitForClone(_initialMint);
        address pos = POSITION_FACTORY.clonePosition(position);
        zchf.registerPosition(pos);
        existing.collateral().transferFrom(msg.sender, address(pos), _initialCollateral);
        IPosition(pos).initializeClone(msg.sender, existing.price(), limit, _initialCollateral, _initialMint);
        return address(pos);
    }

4. Next, mintInternal() and Frankencoin.mint() are subsequently called where Bob is minted usableMint amount of ZCHF after offsetting _feesPPM and _reservePPM:

File: Frankencoin.sol#L165-L170

   function mint(address _target, uint256 _amount, uint32 _reservePPM, uint32 _feesPPM) override external minterOnly {
      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
   }

5. Alice, when attempting to mint some ZCHF, runs into DoS because of mintInternal() reverting on the following code line because minted now equals 10_000:

File: Position.sol#L194

        if (minted + amount > limit) revert LimitExceeded();

Tools Used

Manual

Recommended Mitigation Steps

Consider charging proportionate opening fee to clone users so that the service or facility will not be easily taken granted for. Where possible, reserve a fixed portion of limit to the original position owner.

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L252-L276

Vulnerability details

Impact

Two challenges launched at or about the same time by two or more different challengers could only survive one when all of them have been successful.

Proof of Concept

Here is the scenario:

1. Bob and Alex launch their separate identical challenge on the same position at the same time with Bob slightly earlier than Alex.
2. Both challenges are successful where Alex's challenge.bid is lower than Bob's.
3. While Bob and his challenge.bidder are lingering, Alex's challenge.bidder races to call end() to reap the best deal.
4. Bob loses his challenge reward. The position owner suffers a bigger loss. Bob's challenge.bidder misses out a deal.

Tools Used

Manual

Recommended Mitigation Steps

Consider setting a cooldown on each challenge allowing it to call end() only when there are no higher bidders from other existing challenges that have ended during the cooldown period.

Inadequate calculateFreedAmount() and calculateAssignedReserve()

In Frankencoin.calculateFreedAmount(), users will have to separately figure out on their own what exact amountExcludingReserve is needed in order to fully repay their loans via burnWithReserve() when currentReserve < minterReserve_. Consider refactoring calculateFreedAmount() that will take in another parameter, i.e. the intended freedAmount and correspondingly returns an additional variable, i.e, the required _amountExcludingReserve.

The same refactor consideration should also be given to Frankencoin.calculateAssignedReserve() so owner will know what additional ZCHF, i.e the loss, she will need to have in her wallet prior to calling burnFrom().

Chain reorganization attack

As denoted in the Moralis academy article:

"... If a node receives a new chain that’s longer than its current active chain of blocks, it will do a chain reorg to adopt the new chain, regardless of how long it is."

Depending on the outcome, if it ended up placing the transaction earlier than anticipated, many of the system function calls could backfire. For instance, a bidder bidding on a trimmed position could run into DoS because of size mismatch. Similarly, a postion owner attempting to mint more ZCHF could also run into DoS due to cooldown not over yet etc.

(Note: On Ethereum this is unlikely but this is meant for contracts going to be deployed on any compatible EVM chain many of which like Polygon, Optimism, Arbitrum are frequently reorganized.)

Typo mistakes

File: Position.sol#L358

-     * This is also a good creterion when deciding whether it should be shown in a frontend.
+     * This is also a good criterion when deciding whether it should be shown in a frontend.

File: Equity.sol#L34

-     * I.e., the supply is proporational to the cubic root of the reserve and the price is proportional to the
+     * I.e., the supply is proportional to the cubic root of the reserve and the price is proportional to the

File: Equity.sol#L73

-     * cap of 3,000,000,000,000,000,000 CHF. This limit could in theory be reached in times of hyper inflaction.
+     * cap of 3,000,000,000,000,000,000 CHF. This limit could in theory be reached in times of hyper inflation.

File: Equity.sol#L207

-     * helpes are necessary and to identify them by scanning the blockchain for Delegation events.
+     * helpers are necessary and to identify them by scanning the blockchain for Delegation events.

Include descriptive variables in mappings

Where possible, implement mappings with descriptive variables included for better readability.

Here are some of the instances entailed:

File: MintingHub.sol#L37

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

File: Frankencoin.sol#L45-L50

   mapping (address => uint256) public minters;

   /**
    * List of positions that are allowed to mint and the minter that registered them.
    */
   mapping (address => address) public positions;

Non-compliant contract layout with Solidity's Style Guide

According to Solidity's Style Guide below:

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

In order to help readers identify which functions they can call, and find the constructor and fallback definitions more easily, functions should be grouped according to their visibility and ordered in the following manner:

constructor, receive function (if exists), fallback function (if exists), external, public, internal, private

And, within a grouping, place the view and pure functions last.

Additionally, inside each contract, library or interface, use the following order:

type declarations, state variables, events, modifiers, functions

Consider adhering to the above guidelines for all contract instances entailed.

Tokens accidentally sent to the contract cannot be recovered

It is deemed unrecoverable if any of the ERC20 tokens accidentally arrive at the contract addresses, which has happened to many popular projects. Consider adding a recovery code to your critical contracts just like it has been implemented in contract Position:

File: Position.sol#L245-L255

    /**
     * Withdraw any ERC20 token that might have ended up on this address.
     * Withdrawing collateral is subject to the same restrictions as withdrawCollateral(...).
     */
    function withdraw(address token, address target, uint256 amount) external onlyOwner {
        if (token == address(collateral)){
            withdrawCollateral(target, amount);
        } else {
            IERC20(token).transfer(target, amount);
        }
    }

Parameterized custom errors

Custom errors can be parameterized to better reflect their respective error messages if need be.

For instance, the custom error instance below may be refactored as follows:

File: Position.sol#L103

-    error TooLate();
+    error TooLate(uint256 _start);

File: Position.sol#L110

-        if (block.timestamp >= start) revert TooLate();
+        if (block.timestamp >= start) revert TooLate(start);

Position owner could shill bid after increasing the price

A position owner attempting to mint more ZCHF by increasing the price without increasing the collateral could technically shill bid any challenge launched against it. This will ensure the owner take back his collateral above market price at the worst and per chance the owner could profit from a higher bid than what she has shilled or even have the challenged averted by the next bidders.

Hardcode 7 days

In MintingHub.sol, _initPeriodSeconds, i.e. initPeriod has been harcoded to 7 days. It might seem appropriate or optimized for now but could be too short or too long when market changed and/or more minters surfaced, making this specific input parameter no longer competitive or popular.

Consider soft coding it where possible.

Incorrect parameter on isPosition()

isPosition(spender) generally returns zero address since it is unlikely that the minter (spender) registers itself as a position. It is non-critical here since the first condition should always suffice. Nevertheless, consider having the affected code refactored as follows unless the protocol has an intended plan other than the one aforementioned:

File: Frankencoin.sol#L102-L111

   function allowanceInternal(address owner, address spender) internal view override returns (uint256) {
      uint256 explicit = super.allowanceInternal(owner, spender);
      if (explicit > 0){
         return explicit; // don't waste gas checking minter
-      } else if (isMinter(spender) || isMinter(isPosition(spender))){
+      } else if (isMinter(spender) || isMinter(isPosition(owner))){
         return INFINITY;
      } else {
         return 0;
      }
   }

Sanity checks at the constructor

Adequate zero address, zero value and boundary checks should be implemented at the constructor particularly in Position.sol considering a single casual mistake could end up costing 1000 ZCHF of OPENING_FEE. Misentering a 0 limit, having mintingFeePPM and reserveContribution 1e18 instead of PPM scaled etc are just some of the costly mistakes that could transpire particularly when the user is interacting from a non-UI contract.

Frankencoin contract owner possesses too sensitive privileges

The deployer of the contract has too many privileges relative to standard users. The consequence is disastrous if the contract owner's private key has been compromised.

For a Frankencoin Project project this grand, it increases the likelihood that the owner will be targeted by an attacker, especially given the insufficient protection on sensitive owner private keys. The concentration of privileges creates a single point of failure, specifically on calling mint() to mess up with totalSupply() on the healthy circulation of ZCHF, granting minter role to any party at his/her discretion etc.

Consider:

1. splitting privileges (e.g. via the multisig option) to ensure that no one address has excessive ownership of the system,
2. clearly documenting the functions and implementations the owner can change,
3. documenting the risks associated with privileged users and single points of failure, and
4. ensuring that users are aware of all the risks associated with the system.

Comment and code mismatch

In Position.sol, the NatSpec comment of getUsableMint() says that the non-usable mint is used to buy reserve pool shares, whereas zchf.mint(target, amount, reserveContribution, calculateCurrentFee()) does not have evidence alleging that FPS shares are being bought:

File: Frankencoin.sol#L165-L170

   function mint(address _target, uint256 _amount, uint32 _reservePPM, uint32 _feesPPM) override external minterOnly {
      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
   }

As can be seen from the code block above, rest goes to equity as reserves or as fees only. It is not being routed through transferAndCall() to invoke IERC677Receiver(recipient).onTokenTransfer(msg.sender, amount, data) for minting of FPS. Neither is redeem() called to burn any FPS when calculateAssignedReserve() or freedAmount - _amountExcludingReserve is transferred back to the position owner.

Correct placement of chf.transferFrom()

Consider moving the critical chf.transferFrom() from mint() to mintInternal() just like it has been done so in burnInternal() just in case it can be bricked dodging the need to send in CHF when minting new ZCHF:

File: StablecoinBridge.sol#L44-L53

    function mint(address target, uint256 amount) public {
-        chf.transferFrom(msg.sender, address(this), amount);
        mintInternal(target, amount);
    }

    function mintInternal(address target, uint256 amount) internal {
        require(block.timestamp <= horizon, "expired");
        require(chf.balanceOf(address(this)) <= limit, "limit");
+        chf.transferFrom(msg.sender, address(this), amount);
        zchf.mint(target, amount);
    }

System pauser

Consider implementing a system pauser on critical contracts particularly when it relates to Frankencoin.sol just in case it has been seriously compromised as far as the minting capabilities are of primary concern.

Missing cancel option on bid() and end()

Consider adding checks on bid() and end() such that the functions will have challenges[_challengeNumber] and challenge.position.challengedAmount deleted when challenge.position.collateral() == 0 while returning challenge.bid to challenge.bidder. This early termination is going to save a huge amount of gas making all futile zero transfers.

Unneeded instant cast

Casting an instant to a contract type is unnecessary and is a waste of gas. Do this only when you are casting the contract address.

Here are some of the instances entailed:

File: Position.sol

111:        IReserve(zchf.reserve()).checkQualified(msg.sender, helpers);

170:        return IERC20(collateral).balanceOf(address(this));

228:        IERC20(zchf).transferFrom(msg.sender, address(this), amount);

209:        IERC20(collateral).transfer(target, amount);

File: MintingHub.sol

142:        IERC20(position.collateral()).transferFrom(msg.sender, address(this), _collateralAmount);

263:            IERC20(zchf).transfer(challenge.bidder, challenge.bid - effectiveBid);

284:        IERC20(collateral).transfer(target, amount);

Unneeded check

In the constructor of Position.sol, initPeriod has been hard coded as 7 days in openPosition() of MintingHub.sol, making the following check unnecessary:

File: Position.sol#L53

-        require(initPeriod >= 3 days); // must be at least three days, recommended to use higher values

Unneeded function

In MintingHub.sol, the following end function may be removed since users can always input false if need be in the other end function. It is not very much a convenient function since users would still need to input _challengeNumber instead of just clicking and go style:

File: MintingHub.sol#L235-L237

-    function end(uint256 _challengeNumber) external {
-        end(_challengeNumber, false);
-    }

State Variables Repeatedly Read Should be Cached

SLOADs cost 100 gas each after the 1st one whereas MLOADs/MSTOREs only incur 3 gas each. As such, storage values read multiple times should be cached in the stack memory the first time (costing only 1 SLOAD) and then re-read from this cache to avoid multiple SLOADs.

For instance, mintingFeePPM and start in the code block below should be cached as follows:

File: Position.sol#L187

+            uint32 _mintingFeePPM = mintingFeePPM; // SLOAD 1
+            uint256 _start = start; // SLOAD 1
-            return uint32(mintingFeePPM - mintingFeePPM * (time - start) / (exp - start));
+            // MLOAD 1, 2, 4 & 4
+            return uint32(_mintingFeePPM - _mintingFeePPM * (time - _start) / (exp - _start));

Unneeded global variable cache

There is negligible gas benefit caching a global variable unless it is entailed in a considerably large loop.

Here is a specific instance entailed:

File: Position.sol#L183

        uint256 time = block.timestamp;

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =).

As an example, consider replacing >= with the strict counterpart > in the following inequality instance:

File: Equity.sol#L136

-        return anchorTime() - voteAnchor[owner] >= MIN_HOLDING_DURATION;
        // Rationale for subtracting 1 on the right side of the inequality:
        // x >= 10; [10, 11, 12, ...]
        // x > 10 - 1 is the same as x > 9; [10, 11, 12 ...]
+        return anchorTime() - voteAnchor[owner] > MIN_HOLDING_DURATION - 1;

Similarly, the <= instance below may be replaced with < as follows:

File: Frankencoin.sol#L141

-      if (balance <= minReserve){
+      if (balance < minReserve + 1){

|| costs less gas than its equivalent &&

Rule of thumb: (x && y) is (!(!x || !y))

Even with the 10k Optimizer enabled: ||, OR conditions cost less than their equivalent &&, AND conditions.

As an example, the instance below may be refactored as follows:

File: Frankencoin.sol#L267

-      if (!isMinter(msg.sender) && !isMinter(positions[msg.sender])) revert NotMinter();
+      if (!(isMinter(msg.sender) || isMinter(positions[msg.sender]))) revert NotMinter();

Private function with embedded modifier reduces contract size

Consider having the logic of a modifier embedded through a private function to reduce contract size if need be. A private visibility that saves more gas on function calls than the internal visibility is adopted because the modifier will only be making this call inside the contract.

For example, the modifier instance below may be refactored as follows:

File: Position.sol#L373-L376

+    function _noCooldown() private view {
+        if (block.timestamp <= cooldown) revert Hot();
+    }

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

Use of named returns for local variables saves gas

You can have further advantages in term 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: Position.sol#L268-L276

-    function internalWithdrawCollateral(address target, uint256 amount) internal returns (uint256) {
+    function internalWithdrawCollateral(address target, uint256 amount) internal returns (uint256 balance) {
        IERC20(collateral).transfer(target, amount);
-        uint256 balance = collateralBalance();
+        balance = collateralBalance();
        if (balance < minimumCollateral){
            cooldown = expiration;
        }
        emitUpdate();
-        return balance;
    }

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.0",
settings: {
  optimizer: {
    enabled: true,
    runs: 1000,
  },
},
},
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

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.