Frankencoin - m9800's results

Lines of code

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

Vulnerability details

Impact

The owner of the position is not going to be able to mint ZCHF as the minting will be restricted.

Proof of Concept

A position can be successfully deployed with a minimumCollateral equal to 0. There's no guard against it. There are no really big issues visible or explained by the protocol related to this decision. An issue related to this decision is that an attacker can challenge the position with a size equal to 0. MintingHub will notify the position contract and there will be no reverts because of the first decision made. (ie: 0<0 is false)

if (size < minimumCollateral && size < collateralBalance()) revert ChallengeTooSmall();

Then after challenging the position, the attacker can automatically bid for the challenge with a _bidAmountZCHF equal to 0, then the mintingHub contract will call tryAvertChallenge() on the position contract and restrict the minting for 1 day. (this is possible because 0>=0 is true)

 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); /// HERE!!
            return true;
        } else {
            return false;
        }
    }  

The attack can be repeated until the position expires, and its cost is only the gas cost of the two transactions involved. For instance, if the position lasts a week, the attack's cost to restrict the owner's minting rights is approximately seven times the gas cost of the challenge and bid transactions.

Tools Used

Recommended Mitigation Steps

Do not restrict minting if no value is being challenged and pay extra attention to the minimuCollateral of a position.

[N01] There's no refund for the bidder when tryAvertChallenge() is true. The bidder could be providing more _bidAmountZCHF than the necessary to buy the collateral from the challenger.

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

[N02] Unused function isChallengeOpen() in mintingHub L239

[N03] Sometimes IERC20(zchf) is used and sometimes zchf is treated directly as an ERC20. The variable zchf is an ERC20 so no need to use IERC20(zchf) and it is better to use the same pattern for better code consistency. some examples: https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L263

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

[N04] It is easier to read 1_000_000 to refer to 1 million than 1000_000. Use 1_000_000. example: https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L265

[N05] using "is" as a prefix for a function is normally used for functions that return a bool. Like for example IsMinter(), https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L293 But isPosition() returns an address, https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L300 It is a little confusing how the output of the function should be handled.

[N06] Using transfer and transferFrom erc20 functions more than a security risk as mentioned in the automated findings, are a restriction for the protocol. This is because, if the system is effective, qualified pool shareholders will veto positions that use, for example, USDT as collateral and no assets will be at risk. The problem is that Frankencoin has no good interest rates for liquidations, so one would typically expect positions to be opened with stablecoins. Restricting the protocol from using one of the biggest stablecoins in the market is not very efficient.