Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Constructors cannot be used in upgradeable logic contracts

wttt

Summary

In an upgradeable contract, using a constructor can cause initialization problems because the constructor is only executed once when the contract is deployed, and in the proxy contract mode, the constructor of the logic contract will not be called. In order to ensure that the contract can be initialized correctly after each upgrade, an initializer should be used instead of a constructor.

Vulnerability Details

According to the project's PreMarketsTest, SystemConfig, CapitalPool, TokenManager, PreMarktes, and DeliveryPlace belong to the logic contract.

Because the principles are the same, I only use the SystemConfig contract to prove:

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/SystemConfig.sol

contract SystemConfig is SystemConfigStorage, Rescuable, ISystemConfig {
constructor() Rescuable() {}
​
/**
* @notice Set base platform fee rate and base referral rate
* @dev Caller must be owner
* @param _basePlatformFeeRate Base platform fee rate, default is 0.5%
* @param _baseReferralRate Base referral rate, default is 30%
*/
function initialize(
uint256 _basePlatformFeeRate,
uint256 _baseReferralRate
) external onlyOwner {
basePlatformFeeRate = _basePlatformFeeRate;
baseReferralRate = _baseReferralRate;
}

https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#initializers

https://docs.openzeppelin.com/upgrades-plugins/1.x/proxies#the-constructor-caveat

In Solidity, code that is inside a constructor or part of a global variable declaration is not part of a deployed contract’s runtime bytecode. This code is executed only once, when the contract instance is deployed. As a consequence of this, the code within a logic contract’s constructor will never be executed in the context of the proxy’s state.

Impact

Modifications to state variables in the constructor of a logic contract will not be reflected in the Proxy contract. For example, if the value of a state variable is modified in the constructor of a logic contract, this modification will not be reflected in the state variable of the Proxy contract. Therefore, constructors cannot be used in upgradeable logic contracts.

Tools Used

Manual review

Recommendations

Modify the Rescuable contract

import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import {PausableUpgradeable} from "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

2.

contract Rescuable is Initializable, OwnableUpgradeable, PausableUpgradeable {
bytes4 private constant TRANSFER_SELECTOR =
bytes4(keccak256(bytes("transfer(address,uint256)")));
bytes4 private constant TRANSFER_FROM_SELECTOR =
bytes4(keccak256(bytes("transferFrom(address,address,uint256)")));
​
/// @dev Event emitted when the pause status is set
event SetPauseStatus(bool status);
​
/// @dev Event emitted when an account is rescued
event Rescue(address to, address token, uint256 amount);
​
/// @dev Error message when the transfer fails
error TransferFailed();
​
/// @dev Error message when the initialization fails
error AlreadyInitialized();
​
/// @notice Initializes the smart contract with the new implementation.
function __Rescuable_init() internal initializer {
__Ownable_init();
__Pausable_init();
}

3.

import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
contract SystemConfig is Initializable, SystemConfigStorage, Rescuable, ISystemConfig {
function initialize(
address initialOwner,
uint256 _basePlatformFeeRate,
uint256 _baseReferralRate
) public initializer {
__Rescuable_init();
transferOwnership(initialOwner);
basePlatformFeeRate = _basePlatformFeeRate;
baseReferralRate = _baseReferralRate;
}
Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

[invalid] finding-Rescuable-upgradeable-initializeOwner

Valid high severity, since `initializeOwner` is not called for proxy contracts and the constructor for each `Rescuable.sol` contract will not be invoked during proxy deployment, this leaves the `owner` for each proxy unitialized allowing potential to withdraw fund from other proxy contracts inheriting `Rescuable.sol` respectively.

Appeal created

0xnevi Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

[invalid] finding-Rescuable-upgradeable-initializeOwner

Valid high severity, since `initializeOwner` is not called for proxy contracts and the constructor for each `Rescuable.sol` contract will not be invoked during proxy deployment, this leaves the `owner` for each proxy unitialized allowing potential to withdraw fund from other proxy contracts inheriting `Rescuable.sol` respectively.

Support

FAQs

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