Frankencoin - KIntern_NA's results

Lines of code

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

Vulnerability details

Impact

The challenger losing their collateral at a cheap price.

Proof of concept

There is no check of the position's price during function launchChallenge. It leads to the owner of the position can front-run, and change the price of the position before the laucnhChallenge transaction. After that, the collateral of the challenger will be bidded at a cheap price. Scenario: 1. Bob has a position with collateral is ETH, and price is 2000. 2. Alice sees this price is too high and call launchChallenge to challenge this position with size = 1 ETH 3. Bob front-run Alice's transaction, call adjust price to change the price to be 1 4. Alice's collateral (1 ETH) will be bidded at price = 1 ZCHF, and Alice will lose 1 ETH to get just 1 ZCHF.

Tool used

Manual Review, Foundry

Recommended Mitigation Steps

Add a param expectedPrice to check the price of position in function launchChallenge:

function launchChallenge(address _positionAddr, uint256 _collateralAmount, uint256 expectedPrice) external validPos(_positionAddr) returns (uint256) {
    IPosition position = IPosition(_positionAddr);
    require(position.price() == expectedPrice);
    
    ...
}

Lines of code

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

Vulnerability details

Impact

A position can't mint any ZCHF tokens if it has been attacked, leads to the owner of this position will lose the opening fee without any benefits. Furthermore, the owner can be unable to withdraw positions because the cooldown is increased.

Proof of concept

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

If a challenge is averted, the position will be restricted minting for 1 day. The problem here is a challenger can both challenge and bid in a single transaction, and the challenger will not lose anything except for the gas fee. After that, the position will be pending for minting, even if its price is valid. If attacker repeats it until expiration (just cost the gas fee 1 time / a day), this position might not mint any ZCHF tokens. Furthermore, cooldown will be increased in function restrictMinting for each time, resulting in owner of position might be unable to withdraw collateral.

Tool used

Manual Review

Recommended Mitigation Steps

Add a restriction to the time allowed for bidding on a challenge (for example, bidding is only allowed after 1 day from the start of the challenge)

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L260 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#L269

Vulnerability details

Impact

The challenge of a position can be unable to resolve, then the position can be permanently unable to mint any ZCHF tokens

Proof of concept

Some tokens, such as USDC, have the ability to block malicious addresses by blacklisting them. In function end of a challenge, bidder of this challenge will get the corresponding collateral from the position. So if the bidder address is a blacklist address of collateral token, end function of the challenge will revert. Then the challenge will be unable to resolve, and the position will be unable to mint tokens.

Tool used

Manual Review

Recommended Mitigation Steps

Using a try/catch statement to transfer collateral tokens to bidders will skip the transfer if the bidder is a blacklisted address.

Lines of code

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

Vulnerability details

Impact

When a position no longer has any collateral balance, the challenges of it will be at risk to be unable to resolve, because of the reversion when transferring of 0 amount in function notifyChallengeSucceeded. This leads to the funds of challenger and bidder being locked.

Proof of concept

When working with ERC20 tokens, some tokens (such as LEND) revert when transferring a zero value amount. In function notifyChallengeSucceeded of Position contract, if collateral balance of the contract is 0, it will trigger internalWithdrawCollateral with 0 of amount (_size = 0). Then it might revert because of transferring a zero value amount.

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

function internalWithdrawCollateral(address target, uint256 amount) internal returns (uint256) {
    IERC20(collateral).transfer(target, amount);
    uint256 balance = collateralBalance();
    if (balance < minimumCollateral){
        cooldown = expiration;
    }
    emitUpdate();
    return balance;
}

Tool used

Manual Review

Recommended Mitigation Steps

Should check whether the amount of tokens is greater than 0 before transferring.

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L247

Vulnerability details

Impact

The funds of users will be stolen after depositing into the equity reserve, because it doesn't allow the sender to specifiy the minimum shares.

Proof of concept

function onTokenTransfer(address from, uint256 amount, bytes calldata) external returns (bool) {
    require(msg.sender == address(zchf), "caller must be zchf");
    uint256 equity = zchf.equity();
    require(equity >= MINIMUM_EQUITY, "insuf equity"); // ensures that the initial deposit is at least 1000 ZCHF

    // Assign 1000 FPS for the initial deposit, calculate the amount otherwise
    uint256 shares = equity <= amount ? 1000 * ONE_DEC18 : calculateSharesInternal(equity - amount, amount);
    _mint(from, shares);
    emit Trade(msg.sender, int(shares), amount, price());

    // limit the total supply to a reasonable amount to guard against overflows with price and vote calculations
    // the 128 bits are 68 bits for magnitude and 60 bits for precision, as calculated in an above comment
    require(totalSupply() < 2**128, "total supply exceeded");
    return true;
}

When depositing into the reserve, the sender will receive shares based on the total shares and the current ZCHF balance of the equity reserve. If the current equity is <= the deposit amount, the sender will receive a fixed amount of 1000 * 1e18 shares.

uint256 shares = equity <= amount ? 1000 * ONE_DEC18 : calculateSharesInternal(equity - amount, amount);

Because the function onTokenTransfer doesn't allow the sender to specifiy the minimum shares, the other holders of reserve can perform an sandwich attack. They can withdraw their ZCHF funds from the reserve before the sender's transfer, causing the equity balance to become less than the MINIMUM_EQUITY threshold. Then the sender will only receive 1000 * 1e18 shares, even if they have deposited a large amount of ZCHF.

Scenario: 1. The current balance of equity reserve is 10 million ZCHF. And Alice owns 95% shares of reserve, example 950000 * 1e18 shares 2. Bob submit a transaction to transfer 2 million ZCHF into reserve. 3. Alice front-run this transaction, withdraw more than 9 million of ZCHF tokens from the pool (she has enough shares to withdraw). Equity balance will be 1 million ZCHF tokens. 4. Since the equity balance is less than Bob's deposit amount, Bob will only receive 1000 * 1e18 shares. 5. As a result, Alice will have significantly more shares than Bob, which would allow Alice to easily take Bob's funds by redeeming the shares.

Tool used

Manual Review

Recommended Mitigation Steps

Allow the sender to specify a min acceptable shares to get