The Standard
The Standard
DeFiHardhat
20,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Incorrect initialize() implementation makes SmartVaultManagerV5 un-upgradeable

t0x1c

Summary & Vulnerability Details

The SmartVaultManagerV5 contract is designed as an upgradeable contract which inherits from OpenZeppelin upgradeable contracts ERC721Upgradeable and OwnableUpgradeable:

contract SmartVaultManagerV5 is ISmartVaultManager, ISmartVaultManagerV2, Initializable, ERC721Upgradeable, OwnableUpgradeable {

Hence as recommended, it makes use of the function initialize() here:

function initialize() initializer public {}

The issue is that when using upgradeable contracts, it is important to implement an initializer which will call the base contract’s initializers in turn -

    1. It does not call __Ownable_init(), which results in the following logic from OwnableUpgradeable to be skipped:

function __Ownable_init() internal onlyInitializing {
__Ownable_init_unchained();
}
function __Ownable_init_unchained() internal onlyInitializing {
_transferOwnership(_msgSender());
}

Therefore, the contract owner stays zero initialized/does not update, and this means any function in the contract with use of onlyOwner is impacted.

    1. It also does not call __ERC721_init(), which results in the following logic from ERC721Upgradeable to be skipped:

/**
* @dev Initializes the contract by setting a `name` and a `symbol` to the token collection.
*/
function __ERC721_init(string memory name_, string memory symbol_) internal onlyInitializing {
__ERC721_init_unchained(name_, symbol_);
}
​
function __ERC721_init_unchained(string memory name_, string memory symbol_) internal onlyInitializing {
ERC721Storage storage $ = _getERC721Storage();
$._name = name_;
$._symbol = symbol_;
}

The initialize() function also lacks a check for access control and hence runs some risk of it being called by an external front-runner.

Impact

The Pool contract is designed to be upgradeable but is actually not upgradeable.

Tools Used

Manual inspection

Recommendations

  • Make the calls to parent initializers

  • Other variables may optionally be also intitialized here

  • Also, best to add a require statement enforcing access control so that the call to initialize() can not be front-run by an external user

function initialize() initializer public { // optionally pass more parameters here which can be used to initialize other variables
require(msg.sender == DEPLOYER_ADDRESS, "Not the correct deployer");
__ERC721_init("The Standard Smart Vault Manager", "TSVAULTMAN");
__Ownable_init();
// add any optional variable initializations here
}
Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

uninitialized-variables

informational/invalid

Support

FAQs

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