sherlock-audit / 2023-06-dinari-judging

5 stars 4 forks source link

0xpinky - Contracts never initialize owner for Ownable2Step.sol #115

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

0xpinky

high

Contracts never initialize owner for Ownable2Step.sol

Summary

OZ's Ownable2Step.sol now requires the owner to be explicitly set rather than assuming msg.sender as before. Since the contracts never call the Ownable.sol contructor _owner is never set and is left as address(0). This makes PriceOracle.sol completely nonfunctional and disables the critical seize and seizeNative functions for both TxBuilderExtention.sol and UniswapExtension.sol.

Note : Based on discussion from Dinari team, it was understood that the contract will use OZ version of : v4.9.2 So, this issue is possible.

Vulnerability Detail

TransferRestrictor.sol

OrderFees.sol

    constructor(address owner, uint64 _perOrderFee, uint64 _percentageFeeRate) {
        // Check percentage fee is less than 100%
        if (_percentageFeeRate >= _ONEHUNDRED_PERCENT) revert FeeTooLarge();

        // Set owner
        _transferOwnership(owner);

        // Initialize fees
        perOrderFee = _perOrderFee;
        percentageFeeRate = _percentageFeeRate;
    }

The Ownable.sol (inherited by Ownable2Step.sol) constructor now requires the owner to be explicitly set rather than always defaulting to msg.sender.

Impact

Non-function of both

TransferRestrictor.sol

OrderFees.sol

Code Snippet

TransferRestrictor.sol

OrderFees.sol

Tool used

Manual Review

Recommendation

Call the Ownable.sol constructor to set the owner