Tadle

Summary

Several contracts in the protocol are designed to be upgradeable. But these contracts don't follow the required upgradeable pattern.

Vulnerability Details

The UpgradeableProxy contract is designed to be a proxy of SystemConfig, PreMarkets, DeliveryPlace, CapitalPool and TokenManager contracts. These contracts are designed to be upgradeable using the Transparent Proxy pattern from OpenZeppelin. The problem is that the most of the contracts don't follow the upgradeable pattern.

The UpgradeableProxyContract uses a constructor. But the constructors are not called during proxy deployment, which leads to uninitialized contract states:

The SystemConfig and TokenManager contracts have initialize function but this function has modifier onlyOwner. The initialize function should have initializer modifier even when the owner is trusted. That ensures the function can only be called once, preventing accidental reinitialization.

Also, all contracts that are upgradeable (UpgradeableProxy, SystemConfig, PreMarkets, DeliveryPlace, CapitalPool and TokenManager) should call in the constructor the _disableInitializers function to prevent the base contract from being initialized again. This is a best practice for upgradeable contracts to ensure that the contract cannot be reinitialized after deployment.

Impact

The use of constructors instead of initialize function in the upgradeable contracts leads to uninitialized contract states, because the constructors are not called during proxy deployment.
Also the lack of call to the _disableInitializers function can lead to re-initialization of the contracts and respectively to loss of funds.

Tools Used

Manual Review

Recommendations

Use initialize function instead of constructor and add a call to the _disableInitializers function in the upgradeable contracts. Also, add initializer modifier to ensure the function is only called once and to set the contract’s initialization state.