OpenZeppelin Ownable.sol now requires owner address must be initialized in constructor

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-amphora/blob/daae020331404647c661ab534d20093c875483e1/core/solidity/contracts/governance/AmphoraProtocolToken.sol#L8

Vulnerability details

Impact

Some contracts has used openzeppelin ownable.sol for using owner related access functions and for initialization of owner in contract. However with recent modification of openzeppelin library the owner needs to be explicitly needs to be set in constructor during the deployment. With current impplmentation in contracts where openzeppelin ownable.sol is used, the owner is defaulted to address(0) due to not being set in constructor during deployement.

Considering for example: In AmphoraProtocolToken.sol, With current impplmentation the owner is defaulted to address(0) due to not being set in constructor during deployement.

File: contracts/governance/AmphoraProtocolToken.sol

8 contract AmphoraProtocolToken is IAmphoraProtocolToken, ERC20VotesComp, Ownable {
9   constructor(
10    address _account,
11    uint256 _initialSupply
12  ) ERC20('Amphora Protocol', 'AMPH') ERC20Permit('Amphora Protocol') {
13    if (_account == address(0)) revert AmphoraProtocolToken_InvalidAddress();
14    if (_initialSupply <= 0) revert AmphoraProtocolToken_InvalidSupply();
15
16    _mint(_account, _initialSupply);
17  }

As seen AmphoraProtocolToken contract does not set owner in constructor therefore the said openzeppelin issue exists here.

The following owner accessed functions will be affected,

File: contracts/governance/AmphoraProtocolToken.sol

  function mint(address _dst, uint256 _rawAmount) public onlyOwner {
    _mint(_dst, _rawAmount);
  }

The OpenZeppelin Ownable.sol has changed recently. In older versions, the account which deployed the contract was automatically set the owner of the contract and onlyOwner would have been used per design. Now, the new update requires the contract owner to be specified explicitly as a constructor argument during deployment.

Openzeppelin reference: click here

Per Openzeppelin Ownable.sol, see how the owner is set in constructor during deployment.

File: contracts/access/Ownable.sol

constructor(address initialOwner) {
    _transferOwnership(initialOwner);
}

This is explained with taking one instance however, other contracts where ownable.sol is used will have same impact.

Proof of Concept

AMPHClaimer.sol

VaultController.sol

AmphoraProtocolToken.sol

CurveMaster.sol

ChainlinkOracleRelay.sol

CTokenOracle.sol

Tools Used

Manual Review

Recommended Mitigation Steps

Initialize the owner in the constructor in contracts.

Assessed type

Access Control

[c4-pre-sort]

minhquanym marked the issue as low quality report

[minhquanym]

Invalid since the OZ version using in the project is 4.8.2

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid