Tomb Fork Oracle review

Hello dear Solidity dev, here is my personal review of the Oracle contract used for the majority of Tomb Forks. This one is taken from the Magik Oracle, but my critics will also apply for 99% of other forks.

This oracle contract does not do much: it simply computes the average price of a token over a fixed period. Very well. We like single concerned small contracts that do only one task, but do it well.

We are all juniors

I cannot refrain from smiling when I see someone advertising himself as a senior Solidity or smart contract developer. Ethereum was launched in 2015. The Solidity language itself is still not yet in its 1.0 version. It's currently 0.8. This means the tech in itself is junior.

We are all currently climbing the learning curve. Some patterns are emerging here and there, but many if not most of them will probably be considered as terrible beginner mistakes in a few years.

I'll appear as quite critical and scoff at many lines in the following contract, but keep this in mind: we are all as junior as smart contract development is.

Also, my critics are usually personal opinions, and I can sometimes be completely wrong. One of my hopes writing this review is to have someone more clever than me pinning me down and explaining how wrong I am, so I can improve, as we all need to, in this emerging web3 space.

So, let's start and review that contract line by line. First, we'll encounter the usual contract dependencies.

Spoiler alert: in my next article, I'll present you with my simplified, optimised version of this Oracle contract. In it, I will simply apply all the critics I'm going to write here.

Context

// MagikOracle.sol


// File: @openzeppelin/contracts/utils/Context.sol

pragma solidity >=0.6.0 <0.8.0;

/*
 * @dev Provides information about the current execution context, including the
 * sender of the transaction and its data. While these are generally available
 * via msg.sender and msg.data, they should not be accessed in such a direct
 * manner, since when dealing with GSN meta-transactions the account sending and
 * paying for execution may not be the actual sender (as far as an application
 * is concerned).
 *
 * This contract is only required for intermediate, library-like contracts.
 */
abstract contract Context {
    function _msgSender() internal view virtual returns (address payable) {
        return msg.sender;
    }

    function _msgData() internal view virtual returns (bytes memory) {
        this; // silence state mutability warning without generating bytecode - see https://github.com/ethereum/solidity/issues/2691
        return msg.data;
    }
}

👎 Useless.

From my understanding, the Context contract is only useful if you overload it with actual functions that indeed handle meta-transactions or the like.

Inserting it in your contract as is, is just a waste of gas.

Ownable

// File: @openzeppelin/contracts/access/Ownable.sol

pragma solidity >=0.6.0 <0.8.0;

/**
 * @dev Contract module which provides a basic access control mechanism, where
 * there is an account (an owner) that can be granted exclusive access to
 * specific functions.
 *
 * By default, the owner account will be the one that deploys the contract. This
 * can later be changed with {transferOwnership}.
 *
 * This module is used through inheritance. It will make available the modifier
 * `onlyOwner`, which can be applied to your functions to restrict their use to
 * the owner.
 */
abstract contract Ownable is Context {
    address private _owner;

    event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);

    /**
     * @dev Initializes the contract setting the deployer as the initial owner.
     */
    constructor () internal {
        address msgSender = _msgSender();
        _owner = msgSender;
        emit OwnershipTransferred(address(0), msgSender);
    }

    /**
     * @dev Returns the address of the current owner.
     */
    function owner() public view virtual returns (address) {
        return _owner;
    }

    /**
     * @dev Throws if called by any account other than the owner.
     */
    modifier onlyOwner() {
        require(owner() == _msgSender(), "Ownable: caller is not the owner");
        _;
    }

    /**
     * @dev Leaves the contract without owner. It will not be possible to call
     * `onlyOwner` functions anymore. Can only be called by the current owner.
     *
     * NOTE: Renouncing ownership will leave the contract without an owner,
     * thereby removing any functionality that is only available to the owner.
     */
    function renounceOwnership() public virtual onlyOwner {
        emit OwnershipTransferred(_owner, address(0));
        _owner = address(0);
    }

    /**
     * @dev Transfers ownership of the contract to a new account (`newOwner`).
     * Can only be called by the current owner.
     */
    function transferOwnership(address newOwner) public virtual onlyOwner {
        require(newOwner != address(0), "Ownable: new owner is the zero address");
        emit OwnershipTransferred(_owner, newOwner);
        _owner = newOwner;
    }
}

👍 Ownable is good. It's a very good practice to start all your smart contracts with it. Even if the owner finally has absolutely no power, it's always nice to get some address to reach out for questions, supports, bug reports, enquiries, partnerships, ...

I repeat: even if owner finally does nothing, make your contract ownable.

Operator

// File: contracts/owner/Operator.sol

pragma solidity 0.6.12;

contract Operator is Context, Ownable {
    address private _operator;

    event OperatorTransferred(address indexed previousOperator, address indexed newOperator);

    constructor() internal {
        _operator = _msgSender();
        emit OperatorTransferred(address(0), _operator);
    }

    function operator() public view returns (address) {
        return _operator;
    }

    modifier onlyOperator() {
        require(_operator == msg.sender, "operator: caller is not the operator");
        _;
    }

    function isOperator() public view returns (bool) {
        return _msgSender() == _operator;
    }

    function transferOperator(address newOperator_) public onlyOwner {
        _transferOperator(newOperator_);
    }

    function _transferOperator(address newOperator_) internal {
        require(newOperator_ != address(0), "operator: zero address given for new operator");
        emit OperatorTransferred(address(0), newOperator_);
        _operator = newOperator_;
    }
}

👎 Why on earth would an oracle contract need an operator?

This kind of oracle is just here to give us the average price of a token over some fixed time span. Why would you want to change anything in that simple use case?

IUniswapV2Pair

// File: contracts/interfaces/IUniswapV2Pair.sol

pragma solidity ^0.6.0;

interface IUniswapV2Pair {
    event Approval(address indexed owner, address indexed spender, uint256 value);
    event Transfer(address indexed from, address indexed to, uint256 value);

    function name() external pure returns (string memory);

    function symbol() external pure returns (string memory);

    function decimals() external pure returns (uint8);

    function totalSupply() external view returns (uint256);

    function balanceOf(address owner) external view returns (uint256);

    function allowance(address owner, address spender) external view returns (uint256);

    function approve(address spender, uint256 value) external returns (bool);

    function transfer(address to, uint256 value) external returns (bool);

    function transferFrom(
        address from,
        address to,
        uint256 value
    ) external returns (bool);

    function DOMAIN_SEPARATOR() external view returns (bytes32);

    function PERMIT_TYPEHASH() external pure returns (bytes32);

    function nonces(address owner) external view returns (uint256);

    function permit(
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external;

    event Mint(address indexed sender, uint256 amount0, uint256 amount1);
    event Burn(address indexed sender, uint256 amount0, uint256 amount1, address indexed to);
    event Swap(address indexed sender, uint256 amount0In, uint256 amount1In, uint256 amount0Out, uint256 amount1Out, address indexed to);
    event Sync(uint112 reserve0, uint112 reserve1);

    function MINIMUM_LIQUIDITY() external pure returns (uint256);

    function factory() external view returns (address);

    function token0() external view returns (address);

    function token1() external view returns (address);

    function getReserves()
        external
        view
        returns (
            uint112 reserve0,
            uint112 reserve1,
            uint32 blockTimestampLast
        );

    function price0CumulativeLast() external view returns (uint256);

    function price1CumulativeLast() external view returns (uint256);

    function kLast() external view returns (uint256);

    function mint(address to) external returns (uint256 liquidity);

    function burn(address to) external returns (uint256 amount0, uint256 amount1);

    function swap(
        uint256 amount0Out,
        uint256 amount1Out,
        address to,
        bytes calldata data
    ) external;

    function skim(address to) external;

    function sync() external;

    function initialize(address, address) external;
}

😐 Ok, this is an interface, so we might not have a lot to say here.

Personally, I don't leave the whole interface when I will only use one of two of the functions in it. Who cares about these 27 functions, if you only use 2 of them in your contract?

I like to let people understand quickly what I am actually using, in that interface so I generally remove all the functions I don't use.

When I write a contract, I also write it for the person I'll never know that is going to come across my contract and spend some of his time trying to figure out how it works.

I like to make my intentions clear by removing all the functions that I don't use in the interfaces I need.

Babylonian

// File: contracts/lib/Babylonian.sol

pragma solidity ^0.6.0;

library Babylonian {
    function sqrt(uint256 y) internal pure returns (uint256 z) {
        if (y > 3) {
            z = y;
            uint256 x = y / 2 + 1;
            while (x < z) {
                z = x;
                x = (y / x + x) / 2;
            }
        } else if (y != 0) {
            z = 1;
        }
        // else z = 0
    }
}

😤 Ok, this one is a dependency of a dependency (FixedPoint).

The truth is, the sqrt() function is used nowhere in the final contract. So these are just 19 useless lines.

Keep in mind that many people will just cut and paste your smart contracts and use them almost without touching them.

The semi-original Tomb contracts were almost duplicated nearly 500 times so far ...

When you put some code for verification in FTMscan (or any other blockscan site) please, could you just clean your mess up before sharing it to the world?

In the next article, we will go a bit further in that 700+ lines smart contract code. The next dependency will be FixedPoint.