ICreditAccount Manual Review Findings

ICA-01M: Improper Disable of Initializers

Type	Severity	Location
Standard Conformity		ICreditAccount.sol:L50

Description:

The CreditAccountStorageV1 contract is inheriting from both OpenZeppelin's initialization model as well as the 0xPhase system's custom proxy initialization model, however, it solely disables the 0xPhase model and does not disable the initializer of OpenZeppelin.

Impact:

As the Initializable::initializer and ProxyInitializable::initialize modifiers are put in use in the same function, the risk factor of this exhibit is minimal. However, future upgrades of the token may not properly disable the initializers and thus be susceptible to an exploitation of this flaw.

Example:

Copy
35abstract contract CreditAccountStorageV1 is
36  Initializable,
37  ERC721Upgradeable,
38  ERC721EnumerableUpgradeable,
39  ERC721BurnableUpgradeable,
40  ProxyInitializable,
41  ICreditAccount
42{
43  IDB public db;
44  Manager public manager;
45
46  CountersUpgradeable.Counter internal _tokenIds;
47
48  /// @notice Disables initialization on the target contract
49  constructor() {
50    _disableInitialization();
51  }
52
53  /// @notice Initializes the credit account contract on version 1
54  /// @param db_ The protocol DB
55  function initializeCreditAccountV1(
56    IDB db_
57  ) external initialize("v1") initializer {

Recommendation:

We advise the Initializable::initializer modifier to be introduced to the CreditAccountStorageV1::constructor, ensuring that the contract properly disables initializations of the base implementation of both systems.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The Initializable::initializer modifier has been properly introduced to the contract's CreditAccountStorageV1::constructor, alleviating this exhibit.