Frankencoin - Breeje's results

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L92-L106 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/PositionFactory.sol#L17

Vulnerability details

Impact

Exploiting involving Stealing of funds.

Proof of Concept

The openPosition function deploys a new position contract using the create, where the address derivation depends only on the arguments passed.

At the same time, some of the chains (Polygon, Optimism, Arbitrum) to which the MintingHub will be deployed are suspicious of the reorg attack.

File: PositionFactory

17:    return address(new Position(_owner, msg.sender, _zchf, _collateral, 
18:    _minCollateral, _initialLimit, initPeriod, _duration, 
19:    _challengePeriod, _mintingFeePPM, _liqPrice, _reserve));

Link to code

Even more, the reorg can be couple of minutes long. So, it is quite enough to create the position and transfer funds to that address, especially when someone uses a script, and not doing it by hand.

Optimistic rollups (Optimism/Arbitrum) are also suspect to reorgs since if someone finds a fraud the blocks will be reverted, even though the user receives a confirmation and already created a position.

If Alice creates a new position by deploying a position, and then sends funds to it. Bob sees that the network block reorg happens and calls openPosition. Thus, it creates position with an address to which Alice sends funds. Then Alices' transactions are executed and Alice transfers funds to Bob's controlled position.

Tools Used

VS Code

Recommended Mitigation Steps

Deploy the position contract via create2 with salt.

Gas Optimization

[GAS-01] Use unchecked keyword for few calculations

There are instances when their is a required check above the calculation which makes it impossible to underflow or overflow. Such calculations should be marked unchecked to save gas. These are the instances which the Bot missed:

Instance (4):

File: ERC20.sol

132:    _approve(sender, msg.sender, currentAllowance - amount);

156:    _balances[sender] -= amount;
157:    _balances[recipient] += amount;

File: Frankencoin.sol

286:    _mint(msg.sender, _amount - reserveLeft);

[GAS-02] Unnecessary function declared

In Ownable.sol, there is a onlyOwner modifier which is used at many places in Position.sol. The modifier makes an unnecessary call to requireOwner function which costs more gas. Instead, the one line logic of requireOwner should be placed inline in onlyOwner modifier to save gas for jumping to the function everytime.

File: Ownable.sol

      function requireOwner(address sender) internal view { 
          if (owner != sender) revert NotOwner();
      }

      modifier onlyOwner() {
          requireOwner(msg.sender);
          _;
      }