Frankencoin - Nyx's results

Lines of code

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

Vulnerability details

Impact

The adjustPrice function allows the position owner to frontrun and adjust the price lower when opening a challenge. This may cause the challenger to sell their collateral at a lower price than expected, putting them in an unfavorable position.

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

When the position owner lowers the price, averting a challenge becomes easier. The challenger would be in an unfavorable position because their collateral can be sold at a lower price than they were expecting.

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) {

if (
            challenge.position.tryAvertChallenge(challenge.size, _bidAmountZCHF)
        ) {
            // bid was high enough, let bidder buy collateral from challenger
            zchf.transferFrom(msg.sender, challenge.challenger, _bidAmountZCHF);
            challenge.position.collateral().transfer(
                msg.sender,
                challenge.size
            );

Proof of Concept

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

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L199-L229

Tools Used

Manual Review

Recommended Mitigation Steps

* @param expectedSize       size verification to guard against frontrunners doing a split-challenge-attack

Like in bid() function, consider adding a price verification parameter.

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L88-L101

Vulnerability details

Impact

/**
     * Adjust this position's limit to give away half of the remaining limit to the clone.
     * Invariant: global limit stays the same.

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L88-L101

The function comment states that the function should adjust the position's limit to give away half of the remaining limit to the clone. However, the current implementation does not reflect this behavior:

uint256 reduction = (limit - minted - _minimum) / 2;
limit -= reduction + _minimum;
return reduction + _minimum;

For example , Expected behavior based on the comment would be:

limit = 1000 minted = 100

remaining_limit = limit - minted = 1000 - 100 = 900
clone_limit = remaining_limit / 2 = 450

However, the current implementation returns a clone_limit of 500 instead of the expected 450.

Limit calculation is wrong and can be advantageous for the clone.

Proof of Concept

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L88-L101

Tools Used

Manual Review

Recommended Mitigation Steps

Low - 1 - Centralization Issue

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

Shareholders can prevent opening any position they want.

Low - 2 - burn() function doesnt check horizon

function mintInternal(address target, uint256 amount) internal {
        require(block.timestamp <= horizon, "expired");
        require(chf.balanceOf(address(this)) <= limit, "limit");
        zchf.mint(target, amount);
    }

The mint() function includes a check to determine whether the bridge is expired or not, but the burn() function does not have a similar check in place.