Frankencoin - 0xWaitress's results

Lines of code

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

Vulnerability details

Impact

restructureCapTable does not enumerate each addressToWipe

Proof of Concept

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

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

Tools Used

Recommended Mitigation Steps

Recommendation

    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/Frankencoin.sol#L280-L288

Vulnerability details

Impact

notifyLoss can be frontrun by redeem

Proof of Concept

notifyLoss immediately transfer zchf from reserve to the minter, reducing the amount of reserve and hence the equity and zchf to claim pershare.

While the deposit has 90 days cooldown before depositor can withdraw, current depositor that passed this cooldown can take advantage of a notifyLoss event by first frontrunning the notifyLoss by redeeming, then re-depositing into the protocol to take advantage of the reducedvalue per share.

notifyLoss can only be called by MintingHub::end, current depositor can bundle redeem + end + deposit, when they see a challenge that is ending in loss for the reserve.

Tools Used

Recommended Mitigation Steps

This is a re-current issue for most defi strategy to account loss in a mev-resistent way, a few possible solutions:

1. create an additional window for MintingHub::end to be called by a whitelist, before it opens up to the public. The whitelist is trusted bot that will call end through private mempool.
2. amortised the loss in the next coming period of time instead in 1 go with a MAX_SPEED.
3. create an withdrawal queue such that the final withdrawal price is dependent on the upcoming equity change(s)

1. Trade event emitted with msg.sender, which is always zchf contract

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

        emit Trade(msg.sender, int(shares), amount, price());

Recommendation change it to from

        emit Trade(from, int(shares), amount, price());

--- \n

2. The MIN_HOLDING_DURATION is set to 90 * 7200 (blocks), which is more than 90 days on the context of ethereum. Ethereum currently have around 7100 blocks only, the configuration would lead to an effective 91 days, instead of 90.

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

     * The minimum holding duration in blocks. You are not allowed to redeem your pool shares if you held them
     * for less than the minimum holding duration at average. For example, if you have two pool shares on your
     * address, one acquired 5 days ago and one acquired 105 days ago, you cannot redeem them as the average
     * holding duration of your shares is only 55 days < 90 days.
     */
    uint256 public constant MIN_HOLDING_DURATION = 90*7200 << BLOCK_TIME_RESOLUTION_BITS; // Set to 5 for local testing

Recommendation: set it to a more precise block number

==== \n

3. Challenger cannot be a contract but EOA, now restricted by the returnPostponedCollateral

    function returnPostponedCollateral(address collateral, address target) external {
        uint256 amount = pendingReturns[collateral][msg.sender];
        delete pendingReturns[collateral][msg.sender];
        IERC20(collateral).transfer(target, amount);
    }

this restricts many gas stations or smart contract to collect postponedCollateral.

Recommendation

    function returnPostponedCollateral(address collateral, address target, address from) external {
        uint256 amount = pendingReturns[collateral][from]; @>audit
        delete pendingReturns[collateral][from]; @>audit
        IERC20(collateral).transfer(target, amount);
    }

4. PositionFactory can exist as a library since it does not have any storage variable

contract PositionFactory {

Recommendation

library PositionFactory {