Frankencoin - rbserver's results

Lines of code

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

Vulnerability details

Impact

When there is less than 1000 ZCHF in equity left, a donor can provide more ZCHF to save the Frankencoin system. Before donating to the reserve, this donor would call the following Equity.restructureCapTable function to wipe the passive FPS holders so she or he would not share the donation with these passive FPS holders. However, the Equity.restructureCapTable function executes address current = addressesToWipe[0] when iterating over each address of addressesToWipe. This means that only the first address of addressesToWipe is wiped. After calling this function, the donor would think that she or he has successfully wiped the passive FPS holders of addressesToWipe so she or he then donates to the reserve. Yet, since only the first passive FPS holder of addressesToWipe is wiped, the donor would unknowingly share the donation with the unwiped passive FPS holders. As a result, the donor does not own the 100% of all FPS shares when she or he should and lose parts of the donation to the passive FPS holders who should be wiped but not.

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

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

Proof of Concept

First, please add the following code in contracts\test\MintingHubTest.sol.

1. Add the following redeem function in the User contract.

    function redeem(address target, uint256 shares) public returns (uint256) {
        return Equity(address(zchf.reserve())).redeem(target, shares);
    }

2. Replace https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/test/MintingHubTest.sol#L25-L52 with the following code.

    User alice;
    User bob;

    // @audit add following code for POC purpose
    User charlie;
    /** */

    address latestPosition;
    uint256 latestChallenge;

    constructor(address hub_, address swap_){
        hub = MintingHub(hub_);
        swap = StablecoinBridge(swap_);
        col = new TestToken("Some Collateral", "COL", uint8(0));
        xchf = swap.chf();
        zchf = swap.zchf();
        alice = new User(zchf);
        bob = new User(zchf);

        // @audit add following code for POC purpose
        charlie = new User(zchf);
        /** */
    }

    function initiateEquity() public {
        require(zchf.reserve().totalSupply() == 0, Strings.toString(zchf.reserve().totalSupply()));
        require(zchf.equity() == 1101000000000000000001, Strings.toString(zchf.equity()));
         // ensure there is at least 25'000 ZCHF in equity
        bob.obtainFrankencoins(swap, 10000 ether);
        bob.invest(1000 ether);
        require(zchf.reserve().totalSupply() == 1000 ether, Strings.toString(zchf.reserve().totalSupply()));
        bob.invest(9000 ether);
        alice.obtainFrankencoins(swap, 15000 ether);
        alice.invest(15000 ether);

        // @audit add following code for POC purpose
        charlie.obtainFrankencoins(swap, 1 ether);
        charlie.invest(1 ether);
        /** */

        require(zchf.equity() > 25000 ether, Strings.toString(zchf.equity()));
    }

3. Add the following restructureToWipeTwoAddressesByAlice and donateByAliceAndRedeemByAliceAndBob functions in the MintingHubTest contract.

    function restructureToWipeTwoAddressesByAlice() public {
        address[] memory empty = new address[](0);

        address[] memory list = new address[](2);
        list[0] = address(charlie);
        list[1] = address(bob);

        // at this moment, there is less than 1000 ZCHF in equity left, and alice wants to provide more ZCHF to save Frankencoin system
        Equity equity = Equity(address(zchf.reserve()));

        // charlie and bob are passive FPS holders and have some FPS tokens at this moment
        require(equity.balanceOf(address(charlie)) > 0);
        require(equity.balanceOf(address(bob)) > 0);

        // before donating to reserve, alice calls Equity.restructureCapTable function to wipe both charlie and bob
        alice.restructure(empty, list);
        zchf.reserve().checkQualified(address(alice), empty);

        // after calling Equity.restructureCapTable function, charlie does not hold any FPS tokens anymore but bob still does
        require(equity.balanceOf(address(charlie)) == 0);
        require(equity.balanceOf(address(bob)) > 0);
    }

    function donateByAliceAndRedeemByAliceAndBob() public {
        Equity equity = Equity(address(zchf.reserve()));

        // alice thought that she has successfully wiped charlie and bob so she donates to reserve
        alice.obtainFrankencoins(swap, 5000 ether);
        alice.transfer(zchf, address(zchf.reserve()), 5000 ether);

        // afterwards, alice is able to redeem and receive positive proceeds
        uint256 aliceProceeds = alice.redeem(address(alice), equity.balanceOf(address(alice)));
        require(aliceProceeds > 0);

        // charlie is unable to redeem and receive positive proceeds
        uint256 charlieProceeds = charlie.redeem(address(charlie), equity.balanceOf(address(charlie)));
        require(charlieProceeds == 0);

        // however, bob is still able to redeem and receive positive proceeds so alice unknowingly shares her donation with bob
        uint256 bobProceeds = bob.redeem(address(bob), equity.balanceOf(address(bob)) - 1 ether - 1);
        require(bobProceeds > 0);
    }

Then, in test\PositionTests.ts, please replace https://github.com/code-423n4/2023-04-frankencoin/blob/main/test/PositionTests.ts#L316-L318 with the following code. This Calling Equity.restructureCapTable function does not wipe all passive FPS holders of addressesToWipe test will pass to demonstrate the described scenario.

        function BNToHexNoPrefix(n) {
            let num0x0X = BN.from(n).toHexString();
            return num0x0X.replace("0x0", "0x");
        }

        // @audit comment out this test for POC purpose
        // it("restructuring", async () => {
        //     await mintingHubTest.restructure();
        // });

        it("Calling Equity.restructureCapTable function does not wipe all passive FPS holders of addressesToWipe", async () => {
            await network.provider.send("hardhat_mine", [BNToHexNoPrefix(90 * 7200 + 1)]);

            // alice, who is the donor, calls Equity.restructureCapTable function to wipe both charlie and bob who are passive FPS holders 
            await mintingHubTest.restructureToWipeTwoAddressesByAlice();

            // however, bob is not wiped successfully so alice unknowingly shares her donation with bob
            await mintingHubTest.donateByAliceAndRedeemByAliceAndBob();
        });

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L313 can be updated to the following code.

            address current = addressesToWipe[i];

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#L97-L101

Vulnerability details

Impact

For an opened position, the position's owner can call the following Position.mint function to mint ZCHF tokens after paying the opening fee, depositing the initial collateral tokens, and passing the initialization period.

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

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

Yet, another user can frontrun the opened position's owner's Position.mint transaction by calling the MintingHub.clonePosition function, which further calls the following Position.reduceLimitForClone function, to use all of the opened position's limit for the cloned position. After such frontrunning, the opened position's limit becomes 0, and its owner cannot mint any ZCHF tokens so she or he basically paid and lost the opening fee for nothing. Moreover, since the deposited initial collateral tokens could be invested elsewhere for some returns during the initialization period, the opened position's owner also bears the opportunity cost associated with the deposited initial collateral tokens that do not provide any returns during the initialization period.

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

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

Proof of Concept

Please replace https://github.com/code-423n4/2023-04-frankencoin/blob/main/test/PositionTests.ts#L100-L139 in test\PositionTests.ts with the following code. This User can frontrun opened position's owner's Position.mint transaction by calling Position.reduceLimitForClone function to reduce opened position's limit to 0 test will pass to demonstrate the described scenario.

        // @audit comment out the next two tests for POC purpose
        // it("get loan after 7 long days", async () => {
        //     // "wait" 7 days...
        //     await ethers.provider.send('evm_increaseTime', [7 * 86_400 + 60]); 
        //     await ethers.provider.send("evm_mine");
        //     // thanks for waiting so long
        //     fLimit = await positionContract.limit();
        //     limit = dec18ToFloat(fLimit);
        //     let amount = 10_000;
        //     expect(amount).to.be.lessThan(limit);

        //     let fAmount = floatToDec18(amount);
        //     let fZCHFBefore = await ZCHFContract.balanceOf(owner);
        //     let expectedAmount = dec18ToFloat(await positionContract.getUsableMint(fAmount, true));
        //     expect(expectedAmount).to.be.eq(8900);
        //     await positionContract.connect(accounts[0]).mint(owner, fAmount);//).to.emit("PositionOpened");
        //     let currentFees = await positionContract.calculateCurrentFee();
        //     expect(currentFees).to.be.eq(10000);
        //     let fZCHFAfter = await ZCHFContract.balanceOf(owner);
        //     let ZCHFMinted = dec18ToFloat( fZCHFAfter.sub(fZCHFBefore) );
        //     expect(expectedAmount).to.be.equal(ZCHFMinted);
        // });
        
        // it("clone position", async () => {
        //     let fInitialCollateralClone = floatToDec18(initialCollateralClone);
        //     let fZCHFAmount = floatToDec18(1000);
        //     // send some collateral and ZCHF to the cloner
        //     await mockVOL.transfer(accounts[1].address, fInitialCollateralClone);
        //     await ZCHFContract.transfer(accounts[1].address, fZCHFAmount);
            
        //     await mockVOL.connect(accounts[1]).approve(mintingHubContract.address, fInitialCollateralClone);
        //     fGlblZCHBalanceOfCloner = await ZCHFContract.balanceOf(accounts[1].address);
        //     let tx = await mintingHubContract.connect(accounts[1]).clonePosition(positionAddr, fInitialCollateralClone, 
        //         fMintAmount);
        //     let rc = await tx.wait();
        //     const topic = '0x591ede549d7e337ac63249acd2d7849532b0a686377bbf0b0cca6c8abd9552f2'; // PositionOpened
        //     const log = rc.logs.find(x => x.topics.indexOf(topic) >= 0);
        //     clonePositionAddr = log.address;
        //     clonePositionContract = await ethers.getContractAt('Position', clonePositionAddr, accounts[1]);
            
        // });
        
        it("User can frontrun opened position's owner's Position.mint transaction by calling Position.reduceLimitForClone function to reduce opened position's limit to 0", async () => {
            await ethers.provider.send('evm_increaseTime', [7 * 86_400 + 60]); 
            await ethers.provider.send("evm_mine");

            let fInitialCollateralClone = floatToDec18(initialCollateral * 11);

            let fZCHFAmount = floatToDec18(1000);
            await mockVOL.transfer(accounts[1].address, fInitialCollateralClone);
            await ZCHFContract.transfer(accounts[1].address, fZCHFAmount);
            
            await mockVOL.connect(accounts[1]).approve(mintingHubContract.address, fInitialCollateralClone);
            fGlblZCHBalanceOfCloner = await ZCHFContract.balanceOf(accounts[1].address);

            // after initialization period, the opened position has a positive limit, and its owner calls Position.mint function to mint ZCHF tokens 
            expect(await positionContract.limit()).to.equal(110000000000000000000000n);

            // a user frontruns the opened position's owner's Position.mint transaction by calling MintingHub.clonePosition function,
            //   which further calls Position.reduceLimitForClone function, to use all of the opened position's limit for the cloned position
            await mintingHubContract.connect(accounts[1]).clonePosition(positionAddr, fInitialCollateralClone, await positionContract.limit());

            // after such frontrunning, the opened position's limit becomes 0
            expect(await positionContract.limit()).to.equal(0);

            // the opened position's owner cannot mint any ZCHF tokens as a result
            await expect(
                positionContract.connect(accounts[0]).mint(owner, 1)
            ).to.be.revertedWithCustomError(positionContract, "LimitExceeded");
        });

Tools Used

VSCode

Recommended Mitigation Steps

The Position contract can be updated to add a state variable that represents a period, which is after start, during which the position's owner can mint ZCHF tokens but the position is not allowed to be cloned; only after such period is passed, the position can be allowed to be cloned. The functions that open a position can then be updated to allow the corresponding position's owner to specify such period.

Lines of code

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

Vulnerability details

Impact

The following Frankencoin.denyMinter function can be called to deny a suggested minter when block.timestamp > minters[_minter] is false. When block.timestamp > minters[_minter] becomes true, calling the Frankencoin.denyMinter function reverts so the corresponding minter cannot be denied. Moreover, besides the Frankencoin.denyMinter function, there is no other function that can be used to deny a minter. Therefore, if a currently approved minter becomes malicious or hacked in the future, there is no way to deny such minter from minting ZCHF tokens when block.timestamp > minters[_minter] is true. When this occurs, such minter can mint much ZCHF tokens when it should be prevented from doing so. As a result, the value of the ZCHF token will be much deflated, which would cause significant value losses to the ZCHF token holders.

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

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

Proof of Concept

The following steps can occur for the described scenario.

1. block.timestamp > minters[_minter] is true currently but the corresponding minter, which was previously approved, is hacked.
2. This protocol and users of this protocol all want to deny such minter and prevent it from minting any more ZCHF tokens.
3. However, since calling the Frankencoin.denyMinter function reverts at this moment, and there is no other function that can be used to deny a minter, such minter cannot be denied.
4. Such minter is able to mint much ZCHF tokens when it should be prevented from doing so.
5. As a result, the value of the ZCHF token is hugely deflated.

Tools Used

VSCode

Recommended Mitigation Steps

A function, which is similar to Frankencoin.denyMinter function, that is only callable by the trusted admin or governance can be added for denying a previously approved minter when block.timestamp > minters[_minter] is true.

QA REPORT

[01] COLLECTING OPENING FEE WHEN OPENING A POSITION CAN BE UNFAIR

The following Position.deny function can be called to immediately expire a freshly proposed position for any reason. The opened position's owner has to pay an opening fee but always faces the risk of having the opened position expired for any reason even though such owner would think that she or he opened a legit position. If the position is indeed legit but is denied, the user essentially paid and lost the opening fee for nothing. To be more fair to such owners and also to encourage users from opening positions, please consider making the owners to pay the opening fee when starting to mint after the initialization period instead of when opening the positions.

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

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

[02] INFINITY IS NOT type(uint256).max

The following INFINITY is set to (1 << 255), which is not type(uint256).max, even though INFINITY is an uint256. This is unlike many other protocols' common practice; for example, as shown below, USDT sets MAX_UINT to 2**256 - 1. Hence, users, who are familar with such common practice, can assume that this protocol's INFINITY is also type(uint256).max and would expect the ERC20.transferFrom function below to decrease the allowance that was set to an amount that is less than type(uint256).max. Yet, if such allowance was set to an amount that is less than type(uint256).max but more than (1 << 255), the allowance will not be decreased when calling the ERC20.transferFrom function below, which can result in unexpectedness when user expects the allowance to decrease but it does not. To avoid such unexpectedness, please consider updating INFINITY to type(uint256).max.

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

    uint256 internal constant INFINITY = (1 << 255);

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/ERC20.sol#L125-L135

    function transferFrom(address sender, address recipient, uint256 amount) external override returns (bool) {
        _transfer(sender, recipient, amount);
        uint256 currentAllowance = allowanceInternal(sender, msg.sender);
        if (currentAllowance < INFINITY){
            // Only decrease the allowance if it was not set to 'infinite'
            // Documented in /doc/infiniteallowance.md
            if (currentAllowance < amount) revert ERC20InsufficientAllowance(sender, currentAllowance, amount);
            _approve(sender, msg.sender, currentAllowance - amount);
        }
        return true;
    }

https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#code#L163

    uint public constant MAX_UINT = 2**256 - 1;

[03] shares CANNOT BE UP TO totalShares - ONE_DEC18 IN Equity.calculateProceeds FUNCTION

Although the following Equity.calculateProceeds function's comment states: make sure there is always at least one share, the shares input cannot be up to totalShares - ONE_DEC18. It can only be up to totalShares - ONE_DEC18 - 1 to satisfy require(shares + ONE_DEC18 < totalShares, "too many shares"). If the comment is correct, this require statement should be updated to require(shares + ONE_DEC18 <= totalShares, "too many shares"). Otherwise, the comment can be updated to make sure there is always at least (ONE_DEC18 + 1) wei shares to be more accurate.

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

    function calculateProceeds(uint256 shares) public view returns (uint256) {
        uint256 totalShares = totalSupply();
        uint256 capital = zchf.equity();
        require(shares + ONE_DEC18 < totalShares, "too many shares"); // make sure there is always at least one share
        uint256 newTotalShares = totalShares - shares;
        uint256 newCapital = _mulD18(capital, _power3(_divD18(newTotalShares, totalShares)));
        return capital - newCapital;
    }

[04] USING uint256 IN Equity.anchorTime FUNCTION CAN BE MORE FUTURE-PROOFED

block.number always increases and can grow even faster especially when the chain becomes more efficient. It is possible that block.number << BLOCK_TIME_RESOLUTION_BITS can become too large for uint64 to hold in the future. If this happens, the following Equity.anchorTime function will return an incorrect value that causes many calculations that rely on anchorTime() to be incorrect. To be more future-proofed, please consider using uint256 instead of uint64 in this function.

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

    function anchorTime() internal view returns (uint64){
        return uint64(block.number << BLOCK_TIME_RESOLUTION_BITS);
    }

[05] REDUNDANT CAST

The following PositionFactory.clonePosition function executes Position clone = Position(createClone(existing.original())) and then return address(clone). However, createClone(existing.original()) is already an address so there is no need to cast it to Position and then return its address. Please consider directly return createClone(existing.original()) for higher efficiency.

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/PositionFactory.sol#L30-L34

    function clonePosition(address _existing) external returns (address) {
        Position existing = Position(_existing);
        Position clone = Position(createClone(existing.original()));
        return address(clone);
    }

[06] UNUSED IMPORTS

The IReserve.sol and Ownable.sol are not used in the MintingHub contract, please consider removing them for better code efficiency.

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L5-L7

import "./IReserve.sol";
...
import "./Ownable.sol";

[07] IMMUTABLES CAN USE SAME NAMING CONVENTION

As shown below, some immutables are named using capital letters and underscores while the other immutables are named using lowercased letters. To be more consistent, please consider using the same naming convention for all immutables.

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

   uint256 public immutable MIN_APPLICATION_PERIOD; // for example 10 days
   ...
   IReserve override public immutable reserve;

[08] type(uint128).max CAN BE USED IN Equity.onTokenTransfer FUNCTION'S require STATEMENT

The following Equity.onTokenTransfer function executes require(totalSupply() < 2**128, "total supply exceeded"). Make the code more readable, please consider updating this require statement to require(totalSupply() <= type(uint128).max, "total supply exceeded").

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

    function onTokenTransfer(address from, uint256 amount, bytes calldata) external returns (bool) {
        ...
        // 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;
    }

[09] 1000_000 CAN BE CODED AS 1_000_000 IN Frankencoin.mint FUNCTION

It is a common practice to separate each 3 digits in a number by an underscore. The 1000_000 used in the Frankencoin.mint function below can be coded as 1_000_000 to improve the code readability.

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

   function mint(address _target, uint256 _amount, uint32 _reservePPM, uint32 _feesPPM) override external minterOnly {
      uint256 usableMint = (_amount * (1000_000 - _feesPPM - _reservePPM)) / 1000_000; // rounding down is fine
      _mint(_target, usableMint);
      _mint(address(reserve), _amount - usableMint); // rest goes to equity as reserves or as fees
      minterReserveE6 += _amount * _reservePPM; // minter reserve must be kept accurately in order to ensure we can get back to exactly 0
   }

[10] WORD TYPING TYPO

The following comment states: This limit could in theory be reached in times of hyper inflaction, where inflaction is mistyped. Please change inflaction to inflation.

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

    /**
     ...
     * cap of 3,000,000,000,000,000,000 CHF. This limit could in theory be reached in times of hyper inflaction. 
     */