Frankencoin - decade's results

Lines of code

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

Vulnerability details

Impact

Incorrect typo in function restructureCapTable() leading to only burning tokens of first address of addressToWipe array arguement.

Proof of Concept

Here, in L313, addressToWipe[0] only takes first address of the array. While ignoring the rest and also since first address's tokens are burned it will fail addressesToWipe array has more than one 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[0];
            _burn(current, balanceOf(current));
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Change address current = addressesToWipe[0]; ==> address current = addressesToWipe[i];

1. Meta Transactions not supported due to removal of Context.

• Removing GSN Context will make protocol unsupportive of meta transactions.
• Meta Transactions require _msgSender() and _msgData() to proceed.
• This will not be fulfilled, and certain products build on top of protocols like Biconomy won't be supports.
• https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/ERC20.sol#L7
• https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Ownable.sol#L6

2. Add a MAX application_fee check in suggestMinter()

• _applicationfee is directly transferred to the reserve. Application fee should have maximim application_fee allowed check to avoid unwanted transfer to reserve or any frontend miscalculations.
• Also _applicationFee should be renamed to priority fee is a any suggestor is willing to importance towards her application.
• https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L83-L90

      _transfer(msg.sender, address(reserve), _applicationFee);

3. Check for _minApplicationPeriod to be greater than zero in constructor.

• _minApplicationPeriod is an important variable that restricts adding of new minter for a stipulated period.
• It is recommeded to check _minApplicationPeriod set to be greater than zero or some hardcoded values, to avoid. https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L59-L62

  constructor(uint256 _minApplicationPeriod) ERC20(18){
      MIN_APPLICATION_PERIOD = _minApplicationPeriod;
      reserve = new Equity(this);
   }

Source : https://github.com/Uniswap/v3-core/blob/main/audits/tob/audit.pdf

4. No ability to update reserve in Frankencoin.sol in an unlikely scenario.

• Although protocol design is not dependent on owner type scenario. It is worth to note, that in an unlikely event of attack. There should be functionality to update reserve in frankencoin by the admin. https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L31

5. Consider Two-step ownership transfer for all Position contracts.

• Two-step ownership transfer prevents of falling of ownership of Position contract to unintended address which can be either malicious, inactive or contract address that are unable to any transaction.
• Currently, Ownable contracts directly transfers ownership of the contract in a single transaction.
• https://github.com/solodit/solodit_discussion/discussions/8403

Gas Optimizations

1. totalSupply() unnecessarily checked twice.

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L83-L85

   function suggestMinter(address _minter, uint256 _applicationPeriod, uint256 _applicationFee, string calldata _message) override external {
      if (_applicationPeriod < MIN_APPLICATION_PERIOD && totalSupply() > 0) revert PeriodTooShort();
      if (_applicationFee < MIN_FEE  && totalSupply() > 0) revert FeeTooLow();

• In a if statement with && operand, if the first check is fails, second is check is not reached and if statement returns false. Hence, putting totalSupply()>0 check first will not require you to put it twice.

2. _minCollateral require check in function openPosition() can be shifted above so that users have to less fee on transaction failure.

• https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L88-L113
• In the openPosition() function, it first deploys a position contract and then checks the below require statement.

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

• If this require statement fails, then transaction fails. But user still have to pay the gas used by the transaction upto that require statement.
• Now, since the position contract is deployed, this gas used fee also increases multifold since the transaction inevitably fails.
• If this require check is placed before the position contract is created, then user would not have to pay that much amount during transaction.

3. No need wrapping createClone() return value inside Position interface in clonePosition() function.

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

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

• createClone() returns address itself. There is not need for wrapping it under Position interface and then return address(clone). You can directly return the address.