Part 2
Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

`_Ownable_init()` is not called in `MarketMakingEngineConfigurationBranch` contract

bube

Summary

The MarketMakingEngineConfigurationBranch contract inherits OwnableUpgradeable from Openzeppelin, but doesn't initialize the owner. This means the contract's owner is the zero address, the contract doesn't have an owner.

Vulnerability Details

The MarketMakingEngineConfigurationBranch contract is OwnableUpgradeable, but doesn't have initialize function that initializes the owner:

contract MarketMakingEngineConfigurationBranch is OwnableUpgradeable

The problem is that using upgradeable contracts such as OwnableUpgradeable, it is required to implement an initialize function that calls the base contract's initialize function. For example, this is done correctly in several other files such as UpgradeBranch (this file is out of scope):

​
function initialize(address owner) external initializer {
@> __Ownable_init(owner);
}
​

The MarketMakingEngineConfigurationBranch contract doesn't call the _Ownable_init function and therefore the following logic from the OwnableUpgradeable contract is not executed:

​
function __Ownable_init(address initialOwner) internal onlyInitializing {
__Ownable_init_unchained(initialOwner);
}
​
function __Ownable_init_unchained(address initialOwner) internal onlyInitializing {
if (initialOwner == address(0)) {
revert OwnableInvalidOwner(address(0));
}
_transferOwnership(initialOwner);
}

Because of that the contract owner is not initialized and it is the zero address. This means that all functions that have onlyOwner modifier will always revert. All functions in the contract have onlyOwner modifier, so the contract can not be used at all.

Let's consider this PoC. For simplicity I made a new contract with only one function and skipped the parameters and function body:

​
contract MarketMakingEngineConfigurationBranch is OwnableUpgradeable {
constructor() {
_disableInitializers();
}
​
function createVault() public onlyOwner {}
}
​

And the test function:

​
function testCreateVault() public {
assertEq(market.owner(), address(0x0));
market.createVault();
}
​

The test shows that the owner address of the MarketMakingEngineConfigurationBranch contract is the zero address and each call to function that has onlyOwner modifier reverts:

​
[11442] MarketMakingEngineConfigurationBranchTest::testCreateVault()
├─ [2314] MarketMakingEngineConfigurationBranch::owner() [staticcall]
│ └─ ← [Return] 0x0000000000000000000000000000000000000000
├─ [0] VM::assertEq(0x0000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000) [staticcall]
│ └─ ← [Return]
├─ [380] MarketMakingEngineConfigurationBranch::createVault()
│ └─ ← [Revert] OwnableUnauthorizedAccount(0x7FA9385bE102a...)
└─ ← [Revert] OwnableUnauthorizedAccount(0x7FA9385bE102a...)
​

Impact

All functions in the MarketMakingEngineConfigurationBranch contract have onlyOwner modifier, but the owner is not set. This means the functions from the MarketMakingEngineConfigurationBranch contract can not be used, because they will revert every time due to OwnableUnauthorizedAccount error.
The impact of that is the unability to execute critical functions for the protocol, some of them are: vaults and markets can not be created, updated or connected, engine, collateral can not be configured, markets can not be paused, unpaused, swap strategies can not be configured, fee recipients can not be set or updated.

Tools Used

Manual Review, Foundry

Recommendations

Implement a initialize function that calls the _Ownable_init function with the owner address parameter.

Updates

Lead Judging Commences

inallhonesty Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

`_Ownable_init()` is not called in `MarketMakingEngineConfigurationBranch` contract

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.