JPYC contest - kirk-baird's results

Lines of code

https://github.com/code-423n4/2022-02-jpyc/blob/main/contracts/v1/Ownable.sol#L51-L54

Vulnerability details

Impact

The transferOwnership(address newOwner) function does not use a safe ownership transfer pattern. The impact of this is that if an invalid newOwner (that is not zero) is passed to the function then all onlyOwner functionality will be lost.

Losing onlyOwner functionality will prevent changing the masterMinter and the whitelister as well losing the ability to update the proxy's implementations.

This applies to both FiatTokenV1.sol and FiatTokenV2.sol.

Proof of Concept

    function transferOwnership(address newOwner) public virtual onlyOwner {
        require(newOwner != address(0), "Ownable: new owner is the zero address");
        _transferOwnership(newOwner);
    }

Recommended Mitigation Steps

Consider adding a two stage ownership transfer model where the current owner first selects a pending owner. Second, the pending owner accepts the ownership which will then transfer the ownership to the pending owner.

    public address pendingOwner;

    function transferOwnershipToPending(address _pendingOwner) public virtual onlyOwner {
        pendingOwner = _pendingOwner; // Allow _pendingOwner == address(0)
    }


    function acceptOwnership() public  {
        require(pendingOwner != address(0), "Ownable: new owner is the zero address");
        pendingOwner = newOwner;
    }

Note we allow _pendingOwner == address(0) as we may want to cancel a pendingOwner transfer and retain the current owner. To do this we do transferOwnershipToPending(address(0).