code423n4 commented 2 years ago

Lines of code

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L731-L734 https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/CNftPriceOracle.sol#L54-L56

Vulnerability details

impact

Both Comptroller\_changeAdmin() and CNftPriceOracle\changeAdmin() are used to set new admins. However, they don’t check whether the new admin is valid. Both of these contracts could lose their admins.

Proof of Concept

newAdmin can be address(0) in these functions.

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L731-L734

    function _changeAdmin(address newAdmin) external {
        require(msg.sender == admin, "Only admin can change the admin");
        admin = newAdmin;
    }

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/CNftPriceOracle.sol#L54-L56

    function changeAdmin(address newAdmin) external onlyAdmin {
        admin = newAdmin;
    }

Tools Used

vim

Recommended Mitigation Steps

Add a check like https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol

    function _changeAdmin(address newAdmin) external {
        require(newAdmin != address(0), "new admin cannot be the zero address");
        require(msg.sender == admin, "Only admin can change the admin");
        admin = newAdmin;
    }

    function changeAdmin(address newAdmin) external onlyAdmin {
        require(newAdmin != address(0), "new admin cannot be the zero address");
        admin = newAdmin;
    }

bunkerfinance-dev commented 2 years ago

Duplicate of #58

gzeoneth commented 2 years ago

Downgrading to Low / QA.