MohammedRizwan - `Rewards.sol` has utilized vulnerable openzeppelin `4.1.0` UUPS implementation which has critical vulnerability

[sherlock-admin4]

MohammedRizwan

medium

Rewards.sol has utilized vulnerable openzeppelin 4.1.0 UUPS implementation which has critical vulnerability

Summary

Rewards.sol has utilized vulnerable openzeppelin 4.1.0 UUPS implementation which has critical vulnerability

Vulnerability Detail

Openzeppelin has found the critical severity bug in UUPSUpgradeable. The nouns dao contracts has used both openzeppelin contracts as well as openzeppelin upgrabable contracts with version v4.1.0. This is confirmed from package.json and can be checked here

According to openzeppelin, The UUPSUpgradeable vulnerability has been found in openzeppelin version as follows,

@openzeppelin/contracts : Affected versions >= 4.1.0 < 4.3.2 @openzeppelin/contracts-upgradeable : >= 4.1.0 < 4.3.2

Openzeppelin has fixed this issue in versions 4.3.2. However, Nouns dao contracts falls in above affected versions.

Openzeppelin bug acceptance and fix: check here

Rewards.sol contract has been affected due to this vulnerability as Rewards.sol is an UUPSUpgradeable contract.

contract Rewards is
    UUPSUpgradeable,
    PausableUpgradeable,
    OwnableUpgradeable,
    ERC721Upgradeable,
    INounsClientTokenTypes
{

   . . . some comment
}

Rewards.sol utilizes this vulnerable UUPS implementation and this contains the critical vulnerability that uninitialized implementations can be hijacked and completely bricked.

A detailed vulnerability details can be checked here

A reference exploitable POC for understanding the issue can be checked here

Additional details

According to UUPSUpgradeable Vulnerability Post-mortem which is published by openzeppelin states,

The vulnerability lies in the DELEGATECALL instructions in the upgrade function, exposed by the UUPSUpgradeable base contract. As described here 164, a DELEGATECALL can be exploited by an attacker by having the implementation contract call into another contract that SELFDESTRUCTs itself, causing the caller to be destroyed.

Given an UUPS implementation contract, an attacker can initialize it 173 and appoint themselves as upgrade administrators. This allows them to call the upgradeToAndCall 88 function on the implementation directly, instead of on the proxy, and use it to DELEGATECALL into a malicious contract with a SELFDESTRUCT operation.

If the attack is successful, any proxy contracts backed by this implementation become unusable, as any calls to them are delegated to an address with no executable code. Furthermore, since the upgrade logic resided in the implementation and not the proxy, it is not possible to upgrade the proxy to a valid implementation. This effectively bricks the contract, and impedes access to any assets held on it.

Impact

Upgradeable contracts using UUPSUpgradeable are vulnerable to an attack affecting uninitialized implementation contracts.

Code Snippet

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/client-incentives/Rewards.sol#L36

Tool used

Manual Review

Recommendation

1) Update the openzeppelin library to latest version or preferabley v4.3.0 or above. 2) Check this openzeppelin security advisory to initialize the UUPS implementation contracts. 3) Check this openzeppelin UUPS documentation.

References

1) Similar issue, i had also submitted in Kyberswap audit performed at Sherlock and it can be checked here. This issue was discussed and judged as Medium severity and it can be checked here.

2) Openzeppelin bug acceptance and fix: check here

3) At sherlock, the issue is valid under below rule,

In case the vulnerability exists in a library and an in-scope contract uses it and is affected by this bug this is a valid issue.

4) This comment can be checked for additional reference.

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

WangAudit commented:

invalid; the problem of the bug is that it calls authorizeUpgrade and mitigation by OpenZeppelin is to set onlyOwner modifier; spoiler: it's already implemented and this report is invalid

[jingyi2811]

Escalate

Escalating on behalf of @0xRizwan

[sherlock-admin2]

Escalate

Escalating on behalf of @0xRizwan

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xRizwan]

@WangSecurity

invalid; the problem of the bug is that it calls authorizeUpgrade and mitigation by OpenZeppelin is to set onlyOwner modifier; spoiler: it's already implemented and this report is invalid

This is not completely correct info. How the attacker will make use of upgradeToAndCall() is detailed below and also refer the simple UUPS vulnerability POC attached with this comment.

As mentioned above, re-stating here:

According to UUPSUpgradeable Vulnerability Post-mortem which is published by openzeppelin states,

The vulnerability lies in the DELEGATECALL instructions in the upgrade function, exposed by the UUPSUpgradeable base contract. As described here 164, a DELEGATECALL can be exploited by an attacker by having the implementation contract call into another contract that SELFDESTRUCTs itself, causing the caller to be destroyed.

Given an UUPS implementation contract, an attacker can initialize it 173 and appoint themselves as upgrade administrators. This allows them to call the upgradeToAndCall 88 function on the implementation directly, instead of on the proxy, and use it to DELEGATECALL into a malicious contract with a SELFDESTRUCT operation.

If the attack is successful, any proxy contracts backed by this implementation become unusable, as any calls to them are delegated to an address with no executable code. Furthermore, since the upgrade logic resided in the implementation and not the proxy, it is not possible to upgrade the proxy to a valid implementation. This effectively bricks the contract, and impedes access to any assets held on it.

This UUPS vulnerability is a critical issue informed by openzeppelin and fixed here so the issue is known for openzeppelin libraries as used by projects. However, this protocol couldn't mitigate the issue by upgrading the OZ library to latest version so exposed to this UUPS vulnerability.

Please refer below POC link which is actually referred from this poc and it was submitted to openzeppelin and based on this POC, openzeppelin took action and mitigated this critical issue in OZ version i.e 4.3.2

Nouns protocol has used openzeppelin version 4.1.0 and this version of OZ library is vulnerable according to openzeppelin.

To prove the attack, Please refer the below link for simple UUPS vulnerability exploit:

POC link: https://github.com/0xRizwan/UUPS-Vulnerability

Steps to run POC: 1) Clone the repo 2) run npm install 3) run npx hardhat test 4) ✓ Tests passed

Please note below points with respect to simple POC:

Below is the files structure which should be corelated with Nouns`Rewards.sol,

1) SimpleToken.sol which is an UUPS upgradeable contract..........Ensures same Upgradeable functionality as Rewards.sol 2) ExplodingKitten.sol is the contract used to exploit the UUPS vulnerability. It will help in destroying the implementation i.e SimpleToken.sol 3) SimpleTokenV2.sol is assumed to be an V2 version of SimpleToken.sol which is used to check whether after exploit the contract can be upgradeable...........Assume like V2 of Rewards.sol

1) Both Rewards.sol and SimpleToken.sol has used openzeppelin version 4.1.0 which inherits UUPSUpgradeable so that the issue can be understood easily and it can be corelated with Nounss affectedRewards.sol` contract.

2) BothRewards.sol and SimpleToken.sol has used _authorizeUpgrade() with onlyOwner access modifier in their implementation so point raised by lead judge is void here. Using _authorizeUpgrade() with onlyOwner will not resolve the issue. The issue is present at core openzeppelin library and i think the issue description above is very much detailed to understand it.

3) Both Rewards.sol and SimpleToken.sol has used initialize() with initializer modifier.

In short, the simple POC implementation can be corelated with Nouns's Rewards.sol contract to understand how the openzeppelin UUPS vulnerability can be exploited.

Simple POC result:

  ExplodingKitten
    ✓ should destroy an UUPS proxy with unguarded logic contract irrecoverably (930ms)

  1 passing (933ms)

The issue should be considered valid and it should be high severity as the implementation is being destroyed due to this vulnerability.

Looping @eladmallel for issue visibility

[WangSecurity]

Firstly, the core problem of that vulnerability in general is that when you call public upgradeToAndCall function which calls internal _authorizeUpgrade to change the implementation address. The mitigation by OpenZeppelin is that contracts which inherit from UUPSUpgradeable should override _authorizeUpgrade with OnlyOwner modifier or other access control.

This mirigation is already present in the code here. Therefore, this attack cannot happen on that contract.

As for the "POC" submitted by the Watson, it is irrelevant due to next reasons:

1. The POC is on the arbitrary repo made by the watson himself and has to relation to the Nouns DAO contracts.
2. The contract of ExplodingKitten does NOT have onlyOwner modifier on the _authorizeUpgrade function, therefore, it's different from the Nouns DAO contracts.

And I repeat again, this attack cannot be run on the NounsDAO contract since it already utilizes the mitigation from OpenZeppelin themselves.

[0xRizwan]

@WangSecurity

Respectfully disagree. I would request to recheck the reference links as given above.

Firstly, the core problem of that vulnerability in general is that when you call public upgradeToAndCall function which calls internal _authorizeUpgrade to change the implementation address. The mitigation by OpenZeppelin is that contracts which inherit from UUPSUpgradeable should override _authorizeUpgrade with OnlyOwner modifier or other access control.

Please check the POC to understand the attack flow.

The POC is on the arbitrary repo made by the watson himself and has to relation to the Nouns DAO contracts.

Not true, the POC is showing how UUPS vulnerability can be exploited. It is exactly replicating the Nouns Rewards.sol UUPS upgradeability feature.

The contract of ExplodingKitten does NOT have onlyOwner modifier on the _authorizeUpgrade function, therefore, it's different from the Nouns DAO contracts.

Ser, We are exploiting the SimpleToken.sol as shown in POC. ExplodingKitten is an malicious contract made by attacker and this contract is helping to destroy the implementation via selfDestruct. Therefore, this statement is not correct.

Please check the wormhole exploit which had happended to uninitialized proxy. The issue is very much similar here. https://medium.com/immunefi/wormhole-uninitialized-proxy-bugfix-review-90250c41a43a

[WangSecurity]

Sorry for my misunderstanding. But regarding your test file. Do I understand correctly that in line 48 of your test file the attacker is calling initialize on the SimpleToken? I'm not very good with TS, but as I understand, the attacker calls initialize and then calls upgradeToAndCall and it goes through correctly.

The problem here is that by calling initialize the attacker becomes the owner (due to __Ownable_init(); call), therefore, the onlyOwner check is bypassed cause it's called by the owner.

In that regard, the attack is only possible if the attacker calls initialize function before the actual deployer of the contract. Therefore, this report is invalid due to the rule of front-running initializers.

Again, I want to highlight that the POC is irrelevant cause it has no connection to the NounsDAO code.

Also you can see here recommended mitigation of that vuln by OpenZeppelin.

Thus, the only way for this attack to play out in this codebase is if the owner is malicious or attacker the attacker calla initialize function before the owner, which is invalid under Sherlock's rules.

[0xRizwan]

The issue is valid under below sherlock rule,

In case the vulnerability exists in a library and an in-scope contract uses it and is affected by this bug this is a valid issue.

The initializer rule is not applicable here as that rule is applicable for the damage that is not irreversible but here once the implementation is destroyed by selfDestruct then the damage is irreversible. This is the reason, openzeppelin has identified it as critical issue.

I will leave the issue for head of judging comment and further conclusion.

[WangSecurity]

As I've said above, the issue is already fixed in the Rewards contract as well, therefore, even tho this issue is indeed critical, it can NOT happen in the current contract, cause it's already mitigated.

[0xRizwan]

As I've said above, the issue is already fixed in the Rewards contract as well, therefore, even tho this issue is indeed critical, it can NOT happen in the current contract, cause it's already mitigated.

The issue is not fixed and the fix is to update the openzeppelin version to 4.3.2. You are repeating the same thing. As i said and shown the vulnerability exploit that _authorizeUpgrade() access control mechanism checks if the caller is the owner, but this can be bypassed since ownership of the logic contract was transferred over when an attacker initializes the contract.

_authorizeUpgrade() is not the new functionality with onlyOwner, Please do not consider it a mitigatation to this issue. It was present in vulnerable versions as well as in fixed OZ versions.

Please check this similar UUPS exploit for your reference. This protocol had also _authorizeUpgrade() protected with onlyOwner but still got exploited as i explained the issue above.

Ref: https://etherscan.io/address/0xce03849d56d7bf63c5fea2152488ee6d69684cbf/advanced#code%23F22%23L33

[WangSecurity]

In the above comments I provided a link to OpenZeppelin documentation that explicitly says onlyOwner modifier will fix the issue, you can check it above. In that sense the attack is possible only if the attacker initializes the contract before the owner, which is invalid by the rule of front-running initializers, I've also said it above. Sorry for repeating the same arguments, but they exactly show that this issue is not relevant to this repository, since it's already fixed (again check my previous comments where I left the link to the OpenZeppeling documentation).

[0xRizwan]

Refer https://github.com/OpenZeppelin/openzeppelin-contracts/commit/6241995ad323952e38f8d405103ed994a2dcde8e for the mitigation as given by Openzeppelin. This is the correct way to prevent the UUPS vulnerability and it was proposed in v4.3.2

I think, i have given enough burden of proof and will leave for sherlock head of judging comment.

cc- @eladmallel

[WangSecurity]

The mitigation sent by the Watson is correct, the problem is that it's not the only one. Another mitigation is already present (as said in all the comments above). Leaving this to the head of judging as well.

[eladmallel]

Still suggest this issue be excluded because:

• As mentioned, we have the onlyOwner mitigation in place
• We are practically using v4.4 which has the fix, see our yarn.lock file here

[0xRizwan]

@eladmallel

I think this yarn.lock is the correct file for the inscope contracts, correct me if i am wrong.

As mentioned, we have the onlyOwner mitigation in place

As shown in POC and above followed discussions, This wont fix the UUPS vulnerability, i have already shared a hack above with similar implementation(onlyOwner on authorize function) and was vulnerable to UUPS exploit.

[WangSecurity]

Adding a word here. Such exploit is only viable if the initialize function is not called by the NounsDAO team.

[eladmallel]

sorry, I was wrong earlier to say that onlyOwner mitigates the problem. It does not. however, this issue is still wrong because:

1. the Rewards contract constructor has the initializer modifier, meaning no one can call the initialize function on the implementation contract.
2. as mentioned, we are using a higher version of OZ where the vulnerability is fixed.

[eladmallel]

the yarn.lock file @0xRizwan is pointing to was last updated 3 years ago, and is not the relevant yarn.lock to use when trying to figure out the latest dependencies.

in addition, we import the UUPS contract from contracts-upgradeable which does not even appear in the old yarn.lock @0xRizwan referenced, further showing that it's not relevant for this discussion.

[0xRizwan]

@eladmallel

the Rewards contract constructor has the initializer modifier, meaning no one can call the initialize function on the implementation contract.

Anyone can call initialize() function and can destroy the implementation. The use of initializer modifier is to restrict the repeated initialization of contract.

as mentioned, we are using a higher version of OZ where the vulnerability is fixed.

Package.json confirms the use of OZ v4.1.0 which can be checked here

If the contracts had used higher version where UUPS vulnerability is fixed(> v4.3.2 or above) then it would be clearly seen in package.json, However, this is not the case here. You would need to upgrade the OZ to atleast v4.3.2

[eladmallel]

Hey @0xRizwan please read the OZ code again; the initializer modifier on the constructor solves the problem, I am not sure why you still think it does not? can you try and get more specific?

re:package.json I once again have to push back. In package.json the dep version is specifically: "@openzeppelin/contracts-upgradeable": "^4.1.0". The ^ character means it will install a higher minor version. See this explanation for more info: https://stackoverflow.com/a/22345808

[WangSecurity]

Regarding the first point in the previous message by Elad. Yes, initialize can be called by anyone, but it's safe to assume that the team will do that either in the same transaction or in the very next one. Thus the only probable way is to front run the call to initialise, which is invalid by the rules.

[cvetanovv]

I agree with the comments of the Lead Judge and the Sponsor. The front-running initializer is not a valid issue according to the Sherlock documentation and besides the sponsor explains very well that the version they use does not have this vulnerability.

I planning to reject the escalation and leave the issue as it is.

[0xRizwan]

Hi @cvetanovv

Sherlock rule states,

Front-running initializers: Front-running initializers where there is no irreversible damage or loss of funds & the protocol could just redeploy and initialize again is not a valid issue.

Here, the damage is irreversible damage so i think the above rule is not applicable here. Once the implementation is destroyed by selfDestruct, further upgrades can not be done.

In past such similar issue in Kyberswap audit at sherlock was judged as Medium severity. Please check here

re:package.json I once again have to push back. In package.json the dep version is specifically: "@openzeppelin/contracts-upgradeable": "^4.1.0". The ^ character means it will install a higher minor version. See this explanation for more info: https://stackoverflow.com/a/22345808

The audit was with current scope so valid issues mentioning in current scope should be considered. it will install is a future scenario and should not be applicable in this contest also such exception was not mentioned in readme. Any issues related to v4.1.0 would be considered as an issue as if that library version is vulnerable. Further this issue falls in sherlock valid issue rules as outlined below:

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel.

Further, This rule is applicable here for the validation of this issue.

In case the vulnerability exists in a library and an in-scope contract uses it and is affected by this bug this is a valid issue.

The issue is in library and directly an inscope contract is affected by it. The damage from this attack is irreversible so one should not equate it with normal initializer front running issue, further openzeppelin has identified it as critical issue.

[cvetanovv]

@0xRizwan

First - check this from the docs: "Historical decisions are not considered sources of truth."

Second - try to find the difference between KyberSwap and this one. In KyberSwap the version is exactly defined - Reference. Notice that it is missing ^ character. Now look at how it is in this issue.

The sponsor has explained things very well with this comment and there is nothing more to add.

[Evert0x]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• jingyi2811: rejected