Potential DOS in Contracts Inheriting `UUPSUpgradeable.sol`

[code423n4]

Handle

leastwood

Vulnerability details

Impact

There are a number of contracts which inherit UUPSUpgradeable.sol, namely; GovernanceAction.sol, PauseRouter.sol and NoteERC20.sol. All these contracts are deployed using a proxy pattern whereby the implementation contract is used by the proxy contract for all its logic. The proxy contract will make delegate calls to the implementation contract. This helps to facilitate future upgrades by pointing the proxy contract to a new and upgraded implementation contract. However, if the implementation contract is left uninitialized, it is possible for any user to gain ownership of the onlyOwner role in the implementation contract for NoteERC20.sol. Once the user has ownership they are able to perform an upgrade of the implementation contract's logic contract and delegate call into any arbitrary contract, allowing them to self-destruct the proxy's implementation contract. Consequently, this will prevent all NoteERC20.sol interactions until a new implementation contract is deployed.

Proof of Concept

Initial information about this issue was found here.

Consider the following scenario:

• Notional finance deploys their contracts using their deployment scripts. These deployment scripts leave the implementation contracts uninitialized. Specifically the contract in question is NoteERC20.sol.
• This allows any arbitrary user to call initialize() on the NoteERC20.sol implementation contract.
• Once a user has gained control over NoteERC20.sol's implementation contract, they can bypass the _authorizeUpgrade check used to restrict upgrades to the onlyOwner role.
• The malicious user then calls UUPSUpgradeable.upgradeToAndCall() shown here which in turn calls this function. The new implementation contract then points to their own contract containing a self-destruct call in its fallback function.
• As a result, the implementation contract will be self-destructed due the user controlled delegate call shown here, preventing all future calls to the NoteERC20.sol proxy contract until a new implementation contract has been deployed.

Tools Used

Manual code review

Recommended Mitigation Steps

Consider initializing the implementation contract for NoteERC20.sol and checking the correct permissions before deploying the proxy contract or performing any contract upgrades. This will help to ensure the implementation contract cannot be self-destructed.

[jeffywu]

Acknowledged, I don't think this should be categorized high risk because the worst case is a denial of service and a redeployment of the ERC20 contract. As it stands, we've already successfully deployed our ERC20 contract so this is a non-issue.

I would categorize as 0 (Non-critical)

[sockdrawermoney]

Warden leastwood added this proof of concept to illustrate the vulnerability https://gist.github.com/leastwood/b23d9e975883c817780116c2ceb785b8

[jeffywu]

Ok I retract my previous statement, I misread the issue description. Up to you guys but do you want to pay out a full amount to someone who is reporting issues discovered elsewhere? OZ has already called initialize on our deployed contract for us.

[sockdrawermoney]

@jeffywu I think the question is whether the issue is valid based on the original code base. Given your initial response and change after his proof of concept, my read was there was value here in what he reported. Is that a correct understanding?

[jeffywu]

There was value added here but perhaps not at the same level as the other high risk issues.

[sockdrawermoney]

@jeffywu Thanks for the input. As per our rules, awards are determined strictly based on the judge's assessment of the validity and severity, so we'll see how our judge chooses to score this.