Frankencoin - nobody2018's results

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L140-L148 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L159-L167 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L208-L214

Vulnerability details

Impact

The position owner can keep the position in an unhealthy state to trick users into challenging the position. When a user launches an  challenge, the owner can perform a sandwich attack to the transaction. Call Position.adjust before it to lower price, and then call Position.bid after it to avert the challenge. In this way, the position owner can buy the collateral of all challengers at a low price.

Proof of Concept

Let's assume a scenario. The owner of the position is Bob, and the challenger is Alice. Bob has made his position look unhealthy. In Position contract, the collateral is WETH, price is 2000e18, challengedAmount is 0.  The steps shown below have been front-run by Bob:

1. Bob repays the debt by calling Position.repay, so that the price can be reduced to 100e18. He can also achieve this goal by increasing the collateral. Now price is 100e18.
2. Alice calls MintingHub.launchChallenge. The _collateralAmount param is 10e18. She thought price is 2000e18.
3. Bob calls MintingHub.bid to averts alice's challenge. Now alice received 1000e18 ZCHF, while bob received 10e18 WETH.

As a result, Alice suffered significant losses.

In this way, bob can cause every user who challenges his position to suffer funds loss.

Tools Used

Manual Review

Recommended Mitigation Steps

We should add a _expectedPrice param to the MintingHub.launchChallenge function.

--- a/contracts/MintingHub.sol
+++ b/contracts/MintingHub.sol
@@ -137,8 +137,9 @@ contract MintingHub {
      * @param _collateralAmount  size of the collateral we want to challenge (dec 18)
      * @return index of the challenge in challenge-array
      */
-    function launchChallenge(address _positionAddr, uint256 _collateralAmount) external validPos(_positionAddr) returns (uint256) {
+    function launchChallenge(address _positionAddr, uint256 _collateralAmount, uint256 _expectedPrice) external validPos(_positionAddr) returns (uint256) {
         IPosition position = IPosition(_positionAddr);
+        if (_expectedPrice != position.price()) revert UnexpectedSize();
         IERC20(position.collateral()).transferFrom(msg.sender, address(this), _collateralAmount);
         uint256 pos = challenges.length;

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L329-L354 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L159-L167 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L252-L276 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L280-L288

Vulnerability details

Impact

The attacker mints any amount of ZCHF token.

Proof of Concept

The root of this problem is that the price in Position can be modified to a large number. When a challenge ends, the Position.notifyChallengeSucceeded function will be called to notify that the position has been successfully challenged. Let's take a look at the code for this function:

function notifyChallengeSucceeded(address _bidder, uint256 _bid, uint256 _size) external onlyHub returns (address, uint256, uint256, uint256, uint32) {
        challengedAmount -= _size;
        uint256 colBal = collateralBalance();
        if (_size > colBal){
            _bid = _divD18(_mulD18(_bid, colBal), _size);
            _size = colBal;
        }
        //++++++++++++++   if price is a large number, such as 1e100.		+++++++++++++++++++++++++++++++++++
        uint256 volumeZCHF = _mulD18(price, _size); // How much could have minted with the challenged amount of the collateral
        // The owner does not have to repay (and burn) more than the owner actually minted.  
        uint256 repayment = minted < volumeZCHF ? minted : volumeZCHF; // how much must be burned to make things even

        notifyRepaidInternal(repayment); // we assume the caller takes care of the actual repayment
        internalWithdrawCollateral(_bidder, _size); // transfer collateral to the bidder and emit update
        return (owner, _bid, volumeZCHF, repayment, reserveContribution);
    }

The value of volumeZCHF returned by this function depends on price. The volumeZCHF will be used inside the MintingHub.end function.

function end(uint256 _challengeNumber, bool postponeCollateralReturn) public {
        Challenge storage challenge = challenges[_challengeNumber];
        require(challenge.challenger != address(0x0));
        require(block.timestamp >= challenge.end, "period has not ended");
        // challenge must have been successful, because otherwise it would have immediately ended on placing the winning bid
        returnCollateral(challenge, postponeCollateralReturn);
        // notify the position that will send the collateral to the bidder. If there is no bid, send the collateral to msg.sender
        address recipient = challenge.bidder == address(0x0) ? msg.sender : challenge.bidder;
        //++++++++++++++  volumeZCHF returned by notifyChallengeSucceeded is assigned to volume.   +++++++++++++
        (address owner, uint256 effectiveBid, uint256 volume, uint256 repayment, uint32 reservePPM) = challenge.position.notifyChallengeSucceeded(recipient, challenge.bid, challenge.size);
        if (effectiveBid < challenge.bid) {
            // overbid, return excess amount
            IERC20(zchf).transfer(challenge.bidder, challenge.bid - effectiveBid);
        }
        //++++++++++++++  volume is used to calculate rewards which equals to 2% of volume      +++++++++++++
        uint256 reward = (volume * CHALLENGER_REWARD) / 1000_000;	
        uint256 fundsNeeded = reward + repayment;
        if (effectiveBid > fundsNeeded){
            zchf.transfer(owner, effectiveBid - fundsNeeded);
        } else if (effectiveBid < fundsNeeded){
            //++++++++++++++	mint zchf	+++++++++++++
            zchf.notifyLoss(fundsNeeded - effectiveBid); // ensure we have enough to pay everything
        }
        zchf.transfer(challenge.challenger, reward); // pay out the challenger reward
        zchf.burn(repayment, reservePPM); // Repay the challenged part
        emit ChallengeSucceeded(address(challenge.position), challenge.bid, _challengeNumber);
        delete challenges[_challengeNumber];
    }

From the code above, volumeZCHF returned by Position.notifyChallengeSucceeded is assigned to volume. Two percent of the volume is sent to the challenger as a reward. If the reward is not enough, then call zchf.notifyLoss which will mint enough ZCHF.

function notifyLoss(uint256 _amount) override external minterOnly {
      uint256 reserveLeft = balanceOf(address(reserve));
      if (reserveLeft >= _amount){
         _transfer(address(reserve), msg.sender, _amount);
      } else {
         _transfer(address(reserve), msg.sender, reserveLeft);
         _mint(msg.sender, _amount - reserveLeft);
      }
   }

Now we describe how to mint a huge amount of ZCHF.

1. Bob clones a new position from an existing position and deposits the minimum collateral into this new position. Now the value of price doesn't matter.
2. Bob modifies the price to a large number by Position.adjustPrice, such as 1e100. The price increase will only delay the cooldown for three days.
3. Bob challenges his own position by calling MintingHub.launchChallenge with the _collateralAmount param which equals to  Position.minimumCollateral.
4. Because the price is very high, no one will bid. Having a bid does not affect the result.
5. As time passes, the challenge succeeds. Bob calls MintingHub.end. Then bob gets a huge amount of ZCHF token.

What can bob do with so much ZCHF? He can do anything that will destroy protocol. The easiest thing is to avert all challenges of all positions at any price for the challenger's collateral.

Tools Used

Manual Review

Recommended Mitigation Steps

Fixing this issue is to prevent the price from being modified to an arbitrarily large value. In other words, the owner needs to spend a lot of funds to achieve the goal.

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L313

Vulnerability details

Impact

The Equity.restructureCapTable function mistakenly uses addressesToWipe[0] instead of addressesToWipe[i], which will make only one address to be burned in each tx, resulting in a waste of gas.

Proof of Concept

function restructureCapTable(address[] calldata helpers, address[] calldata addressesToWipe) public {
        require(zchf.equity() < MINIMUM_EQUITY);
        checkQualified(msg.sender, helpers);
        for (uint256 i = 0; i<addressesToWipe.length; i++){
            address current = addressesToWipe[0];	//here
            _burn(current, balanceOf(current));
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

--- a/contracts/Equity.sol
+++ b/contracts/Equity.sol
@@ -310,7 +310,7 @@ contract Equity is ERC20PermitLight, MathUtil, IReserve {
         require(zchf.equity() < MINIMUM_EQUITY);
         checkQualified(msg.sender, helpers);
         for (uint256 i = 0; i<addressesToWipe.length; i++){
-            address current = addressesToWipe[0];
+            address current = addressesToWipe[i];
             _burn(current, balanceOf(current));
         }
     }

Lines of code

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

Vulnerability details

Impact

The owner of the new position waits until the end of initPeriod, and the position cannot be used because the limit is 0. He also pays fee for opening the position.

Proof of Concept

Each new position requires going through a initPeriod time which is bigger than 3 days. When the owner of the new position waits until this period is over, he wants to mint ZCHF.

For convenience, we call this position A. Currently, the minted of A is 0. But Bob cloned A before his operation by calling MintingHub.clonePosition, which internally calls A.reduceLimitForClone to reduce A's limit. Through the comments, we learned that the original intention of this function is to adjust this position's limit to give away half of the remaining limit to the clone. If the _initialMint passed to the MintingHub.clonePosition function is equal to the limit of A, then the limit of A will be updated to 0.

function reduceLimitForClone(uint256 _minimum) external noChallenge noCooldown alive onlyHub returns (uint256) {
    //For new position, minted is 0, if limit is equal to _minimum, then reduction is also 0
        uint256 reduction = (limit - minted - _minimum)/2; // this will fail with an underflow if minimum is too high
        limit -= reduction + _minimum;	//then limit = 0
        return reduction + _minimum;
    }

How does bob clone before the owner operation?

• When the initPeriod ends, the owner does not operate immediately, which gives bob time to clone it.
• Bob can front-run owner's tx.

Tools Used

Manual Review

Recommended Mitigation Steps

We should check that the returned value is within the allowed range before reduceLimitForClone returns.

Lines of code

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

Vulnerability details

Impact

User who clones position does not need to pay fees. This is unfair to the original position's owner.

Proof of Concept

When a user creates a new position, he needs to pay OPENING_FEE ZCHF token as reserve funds. However, there is no fee charged for cloning new position from existing one. The cloned position will reduce the original position's limit, which is the maximum minted amount of the position.

Tools Used

Manual Review

Recommended Mitigation Steps

In my opinion, I think that the user should pay fee to the owner of the cloned contract. The fee is calculated by limit. For example, Position  adds an immutable var called feePerLimit which is calculated in the MintingHub.openPosition function by the formula OPENING_FEE/limit.