Frankencoin - Aymen0909's results

Lines of code

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

Vulnerability details

Impact

Due to an error in the restructureCapTable function only the balance of the first account in the addressesToWipe is burned, so the function will not accomplish the role it was intended to and the active FPS holders who participated in the restructure will be forced to share reserve with the passive FPS holders.

Proof of Concept

The issue occurs in the restructureCapTable function :

File: Equity.sol Line 309-316

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++){
        /** @audit
            only first index is used so only first address will be burned
            It should be addressesToWipe[i] instead of addressesToWipe[0]
        */
        address current = addressesToWipe[0];
        _burn(current, balanceOf(current));
    }
}

The function above is supposed to burn the balances of all the accounts associated with the addresses in the addressesToWipe array, but because only the first index is taken in the for loop addressesToWipe[0] only the first address will get its balance burned and all others addresses balances will remain the same.

This is an intended behaviour as the goal of the function is to remove the passive FPS holders and to give all the shares to FPS holders who participated in the restructure.

Tools Used

Manual review

Recommended Mitigation Steps

The correct index should be used in the restructureCapTable for loop, the function should modified as follows :

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++){
        /** @audit
            Use addressesToWipe[i] instead of addressesToWipe[0]
        */
        address current = addressesToWipe[i];
        _burn(current, balanceOf(current));
    }
}

QA Report

Summary

Findings

1- Collateral invarient not checked in some cases :

Risk : Low

In the Position contract there is a collateral invarient that must always hold and should be checked whenever one of these values changes : price, collateral balance and minted; This is explained in checkCollateral function comments :

File: Position.sol Line 278-281

/**
* This invariant must always hold and must always be checked when any of the three
* variables change in an adverse way.
*/

There are two instances where this invarient is not checked :

1- when the adjustPrice function is called if the new price newPrice is greater than the old price price the invarient is not checked, instead a cooldown period is triggered and the new price is set immediatly.

File: Position.sol Line 159-166

function adjustPrice(uint256 newPrice) public onlyOwner noChallenge {
    /** @audit
        checkCollateral only called if newPrice <= price
    */
    if (newPrice > price) {
        restrictMinting(3 days);
    } else {
        checkCollateral(collateralBalance(), newPrice);
    }
    price = newPrice;
    emitUpdate();
}

2- The notifyChallengeSucceeded function does call the functions notifyRepaidInternal and internalWithdrawCollateral which decreases the values of minted and collateral balance respectively, so the colllateral invarient should be checked but it is not.

File: Position.sol Line 329-354

function notifyChallengeSucceeded(
    address _bidder,
    uint256 _bid,
    uint256 _size
) external onlyHub returns (address, uint256, uint256, uint256, uint32) {
    challengedAmount -= _size;
    uint256 colBal = collateralBalance();
    if (_size > colBal) {
        // Challenge is larger than the position. This can for example happen if there are multiple concurrent
        // challenges that exceed the collateral balance in size. In this case, we need to redimension the bid and
        // tell the caller that a part of the bid needs to be returned to the bidder.
        _bid = _divD18(_mulD18(_bid, colBal), _size);
        _size = colBal;
    }

    // Note that thanks to the collateral invariant, we know that
    //    colBal * price >= minted * ONE_DEC18
    // and that therefore
    //    price >= minted / colbal * E18
    // such that
    //    volumeZCHF = price * size / E18 >= minted * size / colbal
    // So the owner cannot maliciously decrease the price to make volume fall below the proportionate repayment.
    uint256 volumeZCHF = _mulD18(price, _size); // How much could have minted with the challenged amount of the collateral
    // The owner does not have to repay (and burn) more than the owner actually minted.
    uint256 repayment = minted < volumeZCHF ? minted : volumeZCHF; // how much must be burned to make things even

    /** @audit
        `minted` and collateral balance are changed after this calls
    */
    notifyRepaidInternal(repayment); // we assume the caller takes care of the actual repayment
    internalWithdrawCollateral(_bidder, _size); // transfer collateral to the bidder and emit update
    return (owner, _bid, volumeZCHF, repayment, reserveContribution);
}

Mitigation

Make sure that the checkCollateral check is not necessary in the instances aferomentioned, if not the checkCollateral check must be added.

2- Missing zero address check in setOwner function :

Risk : Low

The function setOwner is used to transfer the ownership in the Ownable contract, but the function is missing a zero address check on the new owner address newOwner which could lead to a loss of ownership (it can be burned by accident during the transfer), this issue should be considered and addressed as many contracts of the protocol rely on the Ownable contract for managing ownership.

Mitigation

Consider adding a non zero address check in the setOwner function, or to make it even safer you should user 2-step onwership transfer pattern.

3- No zero address check on sender in _transfer function :

Risk : Low

In the _transfer function inside the ERC20 contract there is no non-zero address check for the sender address even though the rquirements in the function comments indicates that it should not be address(0) :

File: ERC20.sol Line 145-147

* Requirements:
*
* - `sender` cannot be the zero address.

This can potentialy cause problems especially in the transferFrom function as it also does not check the sender` address

Mitigation

Add a non zero address check for the sender address in the _transfer function.

4- Check that mintingFeePPM and reserveContribution are less than 1e6 :

Risk : Low

The variables mintingFeePPM and reserveContribution represent the minting fee and the reserve percentage, both variables are in PPM meaning that 1e6 ≈ 100%, because they are both immutable and thus cannot be changed after deployment the contract should check if their values are indeed less than 1e6 to avoid any problems later.

Mitigation

Add checks in the constructor of the Position contract to verify that the values of mintingFeePPM and reserveContribution are less than 1e6

5- Immutable state variables lack zero address checks :

Constructors should check the values written in an immutable state variables(address) is not the address(0).

Risk : Low

Proof of Concept

Instances include:

File: StablecoinBridge.sol Line 27-28

chf = IERC20(other);
zchf = IFrankencoin(zchfAddress);

File: MintingHub.sol Line 55-56

zchf = IFrankencoin(_zchf);
POSITION_FACTORY = IPositionFactory(factory);

File: Position.sol Line 58-59

zchf = IFrankencoin(_zchf);
collateral = IERC20(_collateral);

Mitigation

Add non-zero address checks in the constructors for the instances aforementioned.

6- Missing Natspec/comments in many functions :

Risk : Non critical

Consider adding natspec/comments on all functions to improve code documentation and readability.

Instances include :

File: Equity.sol

Lines 190-202

Lines 225-233

Lines 266-270

File: Position.sol

Lines 181-189

Lines 292-296

7- Use scientific notation :

When using multiples of 10 you shouldn't use decimal literals or exponentiation (e.g. 1000000, 10**18) but use scientific notation for better readability.

Risk : Non critical

Proof of Concept

Instances include:

File: MathUtil.sol Line 11

uint256 internal constant THRESH_DEC18 =  10000000000000000;//0.01

File: Frankencoin.sol

Line 118

return minterReserveE6 / 1000000;

Line 166

uint256 usableMint = (_amount * (1000_000 - _feesPPM - _reservePPM)) / 1000_000;

Line 205

uint256 theoreticalReserve = _reservePPM * mintedAmount / 1000000;

Line 239

return 1000000 * amountExcludingReserve / (1000000 - adjustedReservePPM);

File: MintingHub.sol

Line 265

uint256 reward = (volume * CHALLENGER_REWARD) / 1000_000;

File: Position.sol

Line 122

return totalMint * (1000_000 - reserveContribution - calculateCurrentFee()) / 1000_000;

Line 124

return totalMint * (1000_000 - reserveContribution) / 1000_000;

Mitigation

Use scientific notation for the instances aforementioned.

Gas Optimizations

Summary

Findings

1- Use memory pointers instead of storage :

Copying structs from storage to memory is cheaper as soon as there are more reads on the storage pointer as there are properties in the struct.

There are 3 instances of this issue :

File: MintingHub.sol

Line 157

Challenge storage challenge = challenges[_challengeNumber];

Line 200

Challenge storage challenge = challenges[_challengeNumber];

Line 253

Challenge storage challenge = challenges[_challengeNumber];

2- storage variable should be cached into memory variables instead of re-reading them :

The instances below point to the second+ access of a state variable within a function, the caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read, thus saves 100gas for each instance.

There are 19 instances of this issue :

File: ERC20.sol Line 155

In the code linked above the value of _balances[sender] is read multiple times (2) from storage and it's value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: Frankencoin.sol

Line 84-85

In the code linked above the value of totalSupply() is read multiple times (2) from storage and it's value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 207-209

In the code linked above the value of minterReserve() is read multiple times (2) from storage and it's value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 294

In the code linked above the value of minters[_minter] is read multiple times (2) from storage and it's value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: MintingHub.sol

Line 158-176

In the code linked above the value of challenge.challenger is read multiple times (3) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 161-176

In the code linked above the value of challenge.position is read multiple times (3) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 171-176

In the code linked above the value of challenge.size is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 201-218

In the code linked above the value of challenge.end is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 202-211

In the code linked above the value of challenge.size is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 203-204

In the code linked above the value of challenge.bid is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 254-272

In the code linked above the value of challenge.challenger is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 259-263

In the code linked above the value of challenge.bidder is read multiple times (3) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 260-274

In the code linked above the value of challenge.bid is read multiple times (3) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 291-294

In the code linked above the value of challenge.challenger is read multiple times (3) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 291-294

In the code linked above the value of challenge.size is read multiple times (3) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: Position.sol

Line 141-142

In the code linked above the value of minted is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 149-150

In the code linked above the value of minted is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 241

In the code linked above the value of minted is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

Line 349

In the code linked above the value of minted is read multiple times (2) from storage and its value does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

3- Multiple address/IDs mappings can be combined into a single mapping of an address/id to a struct :

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There are 2 instances of this issue :

File: Equity.sol Line 83-88

83:     mapping (address => address) public delegates;
88:     mapping (address => uint64) private voteAnchor; 

4- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There are 5 instances of this issue:

File: ERC20.sol Line 273

_balances[sender] -= amount;

In the code linked above the operation cannot underflow due to the checks that preceeds it Line 155 so it should be marked as unchecked.

File: Equity.sol Line 294

uint256 newTotalShares = totalShares - shares;

In the code linked above the operation cannot underflow due to the checks that preceeds it Line 293 so it should be marked as unchecked.

File: Frankencoin.sol Line 144

return balance - minReserve;

In the code linked above the operation cannot underflow due to the checks that preceeds it Line 141 so it should be marked as unchecked.

File: Position.sol

Line 242

minted -= amount;

In the code linked above the operation cannot underflow due to the checks that preceeds it Line 241 so it should be marked as unchecked.

Line 196

minted += amount;

In the code linked above the operation cannot overflow due to the checks that preceeds it Line 194 so it should be marked as unchecked.

5- Input check statements should be placed at the start of the functions :

The check statements on the functions input values should be placed at the beginning of the functions to avoid stack too deep errors and save gas.

There is 1 instance of this issue:

File: MintingHub.sol Line 109

require(_initialCollateral >= _minCollateral, "must start with min col");