No Protection of Uninitialized Implementation Contracts From Attacker

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/2fb1e0ec7a52fc06c2e9c8e561bccba84302e4bb/src/PredyPool.sol#L68
https://github.com/code-423n4/2024-05-predy/blob/2fb1e0ec7a52fc06c2e9c8e561bccba84302e4bb/src/base/BaseHookCallbackUpgradable.sol#L13
https://github.com/code-423n4/2024-05-predy/blob/2fb1e0ec7a52fc06c2e9c8e561bccba84302e4bb/src/base/BaseMarketUpgradable.sol#L36

Vulnerability details

Impact

In the contracts that implement Openzeppelin’s Upgreadable model, uninitialized Implementation contract can be taken over by an attacker with initialize() function.

Proof of Concept

Scenario:

1. Proxy & Implementation are deployed.
2. The Proxy delegates calls to Implementation.initialize() which sets the owner and switches initialized to true in the state of the Proxy.
3. The storage of Implementation however is still intact e.g owner is unset and initialized is false.
4. An attacker calls initialize() directly on Implementation and sets himself as the owner.
5. From here, he has full control to perform any maliceous activities.

Tools Used

Manual Review

Recommended Mitigation Steps

From Openzeppelin Docs:

Do not leave an implementation contract uninitialized. An uninitialized implementation contract can be taken over by an attacker, which may impact the proxy. To prevent the implementation contract from being used, you should invoke the _disableInitializers function in the constructor to automatically lock it when it is deployed:

-   constructor() {}
+   constructor() {
        _disableInitializers();
    }

Assessed type

Other

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Out of scope

[Tigerfrake]

Hello judge,

You have marked this report as Out of scope but what really is described in README.md is not what my report here addresses.

In README.md, this is the problem addressed.

[L-13] Upgradeable contract not initialized
Upgradeable contracts are initialized via an initializer function rather than by a constructor. Leaving such a contract uninitialized may lead to it being taken over by a malicious user

This only requires the initialize() function to be used instead of constructor for contract initialization.

However, my report says that even with the initialize() having initializer modifier, uninitialized Implementation contract can be taken over by an attacker if _disableInitializers(); is not invoked within the constructor.

These two issues are not the same. Kindly reconsider

[alex-ppg]

Hey @Tigerfrake, thanks for contributing to the PJQA process! I advise you to look at my ruling which states that even if it were in scope, there is no HM-level impact arising from this issue.