`PirexRewards` implementation is not initialized, allowing impersonation

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L85-L87

Vulnerability details

Impact

An attacker can become the owner of the PirexRewards contract and try to impersonate the official proxy contract. The fact that the implementation (of which the attacker is the owner) is deployed from the official Pirex address (which will also deploy other Pirex contract) will give credibility to the attacker.

Proof of Concept

The PirexRewards contract is designed to be deployed behind a proxy contract, thus it doesn't have a constructor and implements the initialize function for initialization (PirexRewards.sol#L85-L87):

function initialize() public initializer {
    __Ownable_init();
}

Also, it inherits from OwnerUpgradable, which allows it to have a privileged "owner" role (PirexRewards.sol#L15), which is set in the __Ownable_init function to msg.sender. During deployment, the proxy contract calls the initialize function via delegatecall and sets owner to the deployer (Helper.sol#L127-L132):

PirexRewards pirexRewardsImplementation = new PirexRewards();
TransparentUpgradeableProxy pirexRewardsProxy = new TransparentUpgradeableProxy(
    address(pirexRewardsImplementation),
    PROXY_ADMIN, // Admin address
    abi.encodeWithSelector(PirexRewards.initialize.selector)
);

As a result, the initialize function will be executed in the context of the proxy contract and the owner address will be stored in the storage of the proxy contract. However, the PirexRewards contract will be left uninitialized and its owner will be unset. A malicious actor can call initialize on the PirexRewards contract to initialize the contract and become its owner. Even if the official deployer calls initialize in the PirexRewards, the transaction can still be front-run by a malicious actor.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding a constructor with the initializer modifier:

--- a/src/PirexRewards.sol
+++ b/src/PirexRewards.sol
@@ -82,6 +82,8 @@ contract PirexRewards is OwnableUpgradeable {
     error NotContract();
     error TokenAlreadyAdded();

+    constructor() initializer {}
+
     function initialize() public initializer {
         __Ownable_init();
     }

This constructor will initialize the contract (which can be done only once) but won't set an owner, which will basically make the contract non-functional.

[Picodes]

Finding is valid but there is no break of functionality or loss of funds (although it'd be critical if there was a delegatecall in the contract for example)

[Picodes]

As it's a best practice issue, downgrading to QA

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-11-redactedcartel-findings/issues/277