code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Malicious user can create a new position and pretend the pool as Frakencoin and deceive user to deposit in it.

Proof of Concept

createNewPosition can be called by any user. This function does not have any access modifier. While the comments says Must be called through minting hub to be recognized as valid position, but there are such restriction to call the function.

/**
 * Create a completely new position in a newly deployed contract.
 * Must be called through minting hub to be recognized as valid position.
 */
function createNewPosition(address _owner, address _zchf, address _collateral, uint256 _minCollateral, 
    uint256 _initialLimit, uint256 initPeriod, uint256 _duration, uint256 _challengePeriod, 
    uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reserve) 
    external returns (address) {
    return address(new Position(_owner, msg.sender, _zchf, _collateral, 
        _minCollateral, _initialLimit, initPeriod, _duration, 
        _challengePeriod, _mintingFeePPM, _liqPrice, _reserve));
}

When we see the new position creation flow, createNewPosition is called from MintingHub.sol#L88-L113, minting hub contract is the caller here. https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/MintingHub.sol#L88-L113

In createNewPosition function , for Position, it has the msg.sender as one of the argument. This msg.sender is mintinghub contract here.

       return address(new Position(_owner, msg.sender, _zchf, _collateral,

When looking at the Position contract's constructor,

constructor(address _owner, address _hub, address _zchf, address _collateral, 
    uint256 _minCollateral, uint256 _initialLimit, uint256 initPeriod, uint256 _duration,
    uint256 _challengePeriod, uint32 _mintingFeePPM, uint256 _liqPrice, uint32 _reservePPM) {
    require(initPeriod >= 3 days); // must be at least three days, recommended to use higher values
    setOwner(_owner);
    original = address(this);
    hub = _hub;
    price = _liqPrice;
    zchf = IFrankencoin(_zchf);
    collateral = IERC20(_collateral);
    mintingFeePPM = _mintingFeePPM;
    reserveContribution = _reservePPM;
    minimumCollateral = _minCollateral;
    challengePeriod = _challengePeriod;
    start = block.timestamp + initPeriod; // one week time to deny the position
    cooldown = start;
    expiration = start + _duration;
    limit = _initialLimit;

    emit PositionOpened(_owner, original, _zchf, address(collateral), _liqPrice);
}

the second argument is hub. This mean, Minting hub is only allowed to create new position.

But when we look at the createNewPosition function inside the PositionFactory.sol, there are no restriction such that only the MintingHub should be caller.

Tools Used

Manual code review

Recommended Mitigation Steps

Add access modifier as MintingHub contract. So that only the hub would be called.

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as low quality report

0xA5DF commented 1 year ago

Might be a QA, if the position isn't registered that means nothing for the protocol (and there are always ways to deceive people with fake contracts)

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as primary issue

c4-judge commented 1 year ago

hansfriese marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-04-frankencoin-findings