Frankencoin - deliriusz's results

Lines of code

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

Vulnerability details

Both Position::mint() and Position::withdrawCollateral() require that the position is not actively challenged:

    function mint(address target, uint256 amount) public onlyOwner noChallenge noCooldown alive {
       ...
    }

This makes sense, however launching a challenge is very cheap, making DoSing specific users viable option. Every time that a position is challenged, it has to wait minimum period, or be overbidded to remove it. This results in really cheap and easy challenges, and expensive and cumbersome averts. Hence, I believe that this issue is HIGH, as a user may loose access to their positions as long as an attacker is willing to pay neglegible amounts.

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

Impact

User positions are DoSed

Proof of Concept

Proof of concept for this issue is already a part of test cases. File GeneralTest.t.sol and test case test09EndingChallenge. I'll describe the most interestin parts here:

1. Bob challenges a position with neglegible amount of collateral - 300.

        address posAddress = test04Mint(); // create a position to be challenged
        ...
        uint256 first = bob.challenge(hub, posAddress, 300);

2. Alice averts the challenge

        alice.avertChallenge(hub, swap, first);

3. Internal function looks like below. Alice has to bid whole current value to avert the challenge. She has to have 2x of the capital to avert the challenge.

    function avertChallenge(MintingHub hub, StablecoinBridge swap, uint256 first) public {
        {
            (address challenger, IPosition p, uint256 size, uint256 a, address b, uint256 bid) = hub.challenges(first);
            uint256 amount = size * p.price() / 10 ** 18;
            obtainFrankencoins(swap, amount);
            hub.bid(first, amount, size); // avert challenge
        }
        (address challenger, IPosition p, uint256 size, uint256 a, address b, uint256 bid) = hub.challenges(first);
        require(challenger == address(0x0), "challenge not averted");
        require(!hub.isChallengeOpen(first));
    }

Tools Used

Manual analysis

Recommended Mitigation Steps

Introduce minimum challenge payment, that will make sure that no user can DoS anyone else

Lines of code

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

Vulnerability details

Let's start with the fact that minters are permissionless, so anyone can create their own minter contract and register it in Frankencoin. If it's not challenged in required time, it's functional. Having said that, the only moment that a minter can be removed, is the initial challenge period:

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

After the challenge period is over, there is no possibility to remove it. However, there are many possible issues that may be very harmful:

• minter can go rogue, e.g. using an upgradeable proxy, that after some time is changed to a malicious contract, minting huge amounts of ZCHF tokens
• minter can get compromised - if a minter has an admin address, which gets compromised, it will be used as a tool to mint ZCHF
• minter can contain a bug, that was missed during challenge period. Attackers may use it to their advantage
• minter is no longer needed - e.g. reached limit and noone uses it anymore

Impact

No possibility to stop misbehaving minter

Proof of Concept

I believe that PoC is not required here, as I described the issue enough in the first section

Tools Used

Manual analysis

Recommended Mitigation Steps

Allow denying minters any time. Because it's important role, please consider increasing voting power from 3% to 5%.