Frankencoin - Ruhum's results

Lines of code

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

Vulnerability details

Impact

The function doesn't burn the funds of all the provided addresses. Subsequent depositors trying to bootstrap the system again will receive fewer shares than they expected since the total supply remains almost unchanged.

Proof of Concept

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

It only clears the funds of the address at index 0.

Tools Used

none

Recommended Mitigation Steps

Properly loop over all the addresses:

    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[i];
            _burn(current, balanceOf(current));
        }
    }

Lines of code

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

Vulnerability details

Impact

When cloning a position you can use up the remaining minting limit of it. That will force the owner of that position to create a new one or clone one as well.

Proof of Concept

The reduceLimitForClone() function is supposed to grant half of the position's mint limit to the clone:

    /**
     * Adjust this position's limit to give away half of the remaining limit to the clone.
     * Invariant: global limit stays the same.
     *
     * Cloning a position is only allowed if the position is not challenged, not expired and not in cooldown.
     *
     * @param _minimum amount that clone wants to mint initially
     * @return limit for the clone
     */
    function reduceLimitForClone(uint256 _minimum) external noChallenge noCooldown alive onlyHub returns (uint256) {
        uint256 reduction = (limit - minted - _minimum)/2; // this will fail with an underflow if minimum is too high
        limit -= reduction + _minimum;
        return reduction + _minimum;
    }

But, by setting _minimum so that $minimum = limited - minimum$ you can set limit = minted. Meaning, the position can't mint more ZCHF.

For that, the person cloning has to provide enough collateral to mint the whole amount at the time of cloning.

Tools Used

none

Recommended Mitigation Steps

reduceLimitForClone() should remove the _minimum parameter and always decrease the limit by half. If that amount is not enough to fulfill the caller's initial mint, just revert. Force the user to clone a different position or create their own one.

Lines of code

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

Vulnerability details

Impact

The project is supposed to be self-governing. Token owners are able to suggest new minters and collateral. The auction mechanism allows participants to remove collateral from the system if it's deemed unhealthy. But, there's no way to remove a registered minter.

Why would you want to remove a registered minter? Because they can have bugs that could break the whole system. The current minter, MintingHub, for example, implements a novel auction mechanism to price collateral instead of choosing the industry standard Chainlink oracles. While I support the idea of having a fully decentralized system, it does add additional risk to the project. A risk that's taken not only by the protocol team but every ZCHF holder as well.

Obviously, these are just hypotheticals. But, the system is not equipped to handle a scenario where the minter malfunctions.

Proof of Concept

After a minter is suggested you have generally 10 days to deny it. After that, there's no way to remove it:

   function denyMinter(address _minter, address[] calldata _helpers, string calldata _message) override external {
      if (block.timestamp > minters[_minter]) revert TooLate();
      reserve.checkQualified(msg.sender, _helpers);
      delete minters[_minter];
      emit MinterDenied(_minter, _message);
   }

Tools Used

none

Recommended Mitigation Steps

Implement the ability for token holders to temporarily pause a minter as well as remove it altogether.