The Standard

The Standard
DeFiHardhat
20,000 USDC
View results
Submission Details
Severity: low
Invalid

Contracts are not using their OZ upgradeable counterparts

[L-02] Contracts are not using their OZ upgradeable counterparts

Description
The non-upgradeable standard version of OpenZeppelin's library, such as Ownable, Pausable, Address, Context, SafeERC20, ERC1967Upgrade etc, are inherited / used by both the proxy and the implementation contracts.

As a result, when attempting to use the upgrades plugin mentioned, the following errors are raised:

Error: Contract `FiatTokenV1` is not upgrade safe
contracts/v1/FiatTokenV1.sol:58: Variable `totalSupply_` is assigned an initial value
Move the assignment to the initializer
https://zpl.in/upgrades/error-004
contracts/v1/Pausable.sol:49: Variable `paused` is assigned an initial value
Move the assignment to the initializer
https://zpl.in/upgrades/error-004
contracts/v1/Ownable.sol:28: Contract `Ownable` has a constructor
Define an initializer instead
https://zpl.in/upgrades/error-001
contracts/util/Address.sol:186: Use of delegatecall is not allowed
https://zpl.in/upgrades/error-002

Having reviewed these errors, none had any adversarial impact:

totalSupply_ and paused are explictly assigned the default values 0 and false
the implementation contracts utilises the internal _transferOwnership() in the initializer, thus transferring ownership to newOwner regardless of who the current owner is
Address's delegatecall is only used by the ERC1967Upgrade contract. Comparing both the Address and ERC1967Upgrade contracts against their upgradeable counterparts show similar behaviour (differences are some refactoring done to shift the delegatecall into the ERC1967Upgrade contract).
Nevertheless, it would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.

6 import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L6

5 import "@openzeppelin/contracts/access/Ownable.sol";

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPoolManager.sol#L5

7 import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultManagerV5.sol#L7

5 import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/SmartVaultV3.sol#L5

Recommended Mitigation Steps

Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.

Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

informational/invalid

Support

FAQs

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