code423n4 commented 1 year ago

Lines of code

Vulnerability details

Impact

User defined address should always have zero address check. This checks SHOULD NOT BE MISSED IN CASE OF A FACTORY CONTRACT. This will lead to redeployments of contract and blockage of certain functionality as described below. It is also worth to note that is Data Validation bug was acknowledged by Uniswap v3 and modified. Check here - \ https://github.com/Uniswap/v3-core/blob/main/audits/tob/audit.pdf

Proof of Concept

In constructor() there is no check for zero address. Once it's set to any address, there's no configuration functions to change it.

    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;

Similarly, onTokenTransfer() in StableCoinBridge if constructor address are not set properly and set to address(0), then this function will fail, indefinitely.

    function onTokenTransfer(address from, uint256 amount, bytes calldata) external returns (bool){
        if (msg.sender == address(chf)){
            mintInternal(from, amount);
        } else if (msg.sender == address(zchf)){
            burnInternal(address(this), from, amount);
        } else {
            require(false, "unsupported token");
        }
        return true;
    }

Similarly, no Frankencoin token will be ever transferred to equity.sol as onTokenTransfer() of equity.sol will always fail if constructor is not set correctly. Leading to redeployment of the contract. Also there is no proper way to update the variable in the same contract.

    function onTokenTransfer(address from, uint256 amount, bytes calldata) external returns (bool) {
        require(msg.sender == address(zchf), "caller must be zchf");
        uint256 equity = zchf.equity();

Tools Used

Manual Review

Recommended Mitigation Steps

Apply zero address check to all the above mentioned affected code.

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as duplicate of #935

c4-judge commented 1 year ago

hansfriese marked the issue as unsatisfactory: Invalid

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

Lack of zero address check throughout the codebase could lead to unwanted redeployments, address(0) ownership and `onTokenTransfer` unsuccessful. #933