sherlock-admin commented 1 year ago

MohammedRizwan

high

UUPSUpgradeable vulnerability in OpenZeppelin Contracts

Summary

Vulnerability Detail

Openzeppelin has found the critical severity bug in UUPSUpgradeable. The kyber-swap contracts has used both openzeppelin contracts as well as openzeppelin upgrabable contracts with version v4.3.1. This is confirmed from package.json.

File: ks-elastic-sc/package.json

    "@openzeppelin/contracts": "4.3.1",
    "@openzeppelin/test-helpers": "0.5.6",
    "@openzeppelin/contracts-upgradeable": "4.3.1",

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

However, openzeppelin has fixed this issue in versions 4.3.2

Openzeppelin bug acceptance and fix: check here

The following contracts has been affected due to this vulnerability

1) PoolOracle.sol 2) TokenPositionDescriptor.sol

Both of these contracts are UUPSUpgradeable and the issue must be fixed.

Impact

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

Code Snippet

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/package.json#L35-L37

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/oracle/PoolOracle.sol#L19

https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/periphery/TokenPositionDescriptor.sol#L13

Tool used

Manual Review

Recommendation

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

sherlock-admin commented 1 year ago

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

Trumpero commented:

valid medium about OZ upgradable 4.3.1

manhlx3006 commented 1 year ago

Sponsor Confirmed

The finding is valid and a known issue (we have initialized all Pool Oracle for deployed contracts and transferred ownership to a multisig). The UUPSUpgradable has a vulnerability with the version we used. If the implementation is not initialized, someone can initialize and try to destroy the implementation.
For deployed contracts: We have initialized all implementation of deployed contracts and currently our multisig is the owner of all, thus, for the current deployed contracts, the issue won’t happen.
For the future deployment: we will move to the new OZ version 4.3.2.

nevillehuang commented 1 year ago

Escalate

This should be low severity and invalid according to sherlock rules. This comes down to admin deployment errors where admin deploying PoolOracle contract fails to initialize contracts, which kyberswap admins have already done and in fact are aware of this issue.

It really only affects future deployments, but given kyberswap are moving towards 4.3.2, and no other contracts are going to be deployed with this version of OZ 4.3.1, this is a non-issue as there is no existing risk to the protocol, unless the admin makes an error and mistakenly deploy new contracts with this versions.

sherlock-admin2 commented 1 year ago

Escalate

This should be low severity and invalid according to sherlock rules. This comes down to admin deployment errors where admin deploying PoolOracle contract fails to initialize contracts, which kyberswap admins have already done and in fact are aware of this issue.

It really only affects future deployments, but given kyberswap are moving towards 4.3.2, and no other contracts are going to be deployed with this version of OZ 4.3.1, this is a non-issue as there is no existing risk to the protocol, unless the admin makes an error and mistakenly deploy new contracts with this versions.

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.

jkoppel commented 1 year ago

Escalate

This is out of scope. This is at best a flaw in package.json, which is not in scope. The rules about vulns in libraries were changed during the judging period and should not apply to this contest.

Further, Sherlock has in the past ruled it invalid when there is an issue in the code that the protocol team has already fixed by a deployment strategy. https://github.com/sherlock-audit/2023-02-blueberry-judging/issues/146/#issuecomment-1475484578

sherlock-admin2 commented 1 year ago

Escalate

This is out of scope. This is at best a flaw in package.json, which is not in scope. The rules about vulns in libraries were changed during the judging period and should not apply to this contest.

Further, Sherlock has in the past ruled it invalid when there is an issue in the code that the protocol team has already fixed by a deployment strategy. https://github.com/sherlock-audit/2023-02-blueberry-judging/issues/146/#issuecomment-1475484578

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.

jingyi2811 commented 12 months ago

Escalate

i don't agree with both escalations.

UUPSUpgradeable vulnerability in OpenZeppelin Contracts is a critical vulnerability per the openzeppelin which was found in 2021 and to be noted this was the only second critical issue found till date in openzeppelin library. If the implementation is not initialized, someone can initialize and try to destroy the implementation which i agree with the sponsor comment here.

It should be noted here that TokenPositionDescriptor.sol as well as PoolOracle.sol is affected from this issue. TokenPositionDescriptor.sol is also in scope contract. As sponsor confirmed the issue with reasoning and judge reasoning for this issue confirmation too, This is absolutely a valid issue. Below is the additioanal context for raised escalations.

Issues found in used library dependencies has been judged as Medium at sherlock in past irrespective of new judging guidlines. Below are few issues for references,

1) Harpie audit, check Issue M-6: Signature malleability not protected against---Similar openzeppelin library bug issue judged as Medium, However the library issue was High severity and here it is critical severity.

2) UXD audit, check FullMath library overflow issue----library issue taken from uniswap

3) Bond audit, check Solmate code size issue-----solmate library token code size issue judged as Medium

4) I wouldn't had added this reference but i think it should be as the escalations focuses on rules and past issues. Check this issue. The issue is different but the escalation decision revolves around the used library where the Sherlock team reasoning for escalation rejection was

The library is used in the in-scope contract and the error impacts an in-scope contract. This is a valid issue.

To be noted, all these issues were happened before July, 2023 and the new rules made significant judging changes after July, 2023.(This was not intended to add but to respond a escalation, It was necessary to add for reference purpose only)

Per the sherlock rules too, This finding is valid and Sherlock rules states,

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.

Sherlock docs reference here

Contest readme.md also dont have this as known issue even the previous audits done by kyberSwap team does not highlight this critical issue too. I believe this issue is correctly judged by the lead judge and it's good that this issue is being found at sherlock which was missed in previous audits.

I hope, the above explanation is good enough for the validation of this issue.

Thank you!

sherlock-admin2 commented 12 months ago

Escalate

i don't agree with both escalations.

UUPSUpgradeable vulnerability in OpenZeppelin Contracts is a critical vulnerability per the openzeppelin which was found in 2021 and to be noted this was the only second critical issue found till date in openzeppelin library. If the implementation is not initialized, someone can initialize and try to destroy the implementation which i agree with the sponsor comment here.

It should be noted here that TokenPositionDescriptor.sol as well as PoolOracle.sol is affected from this issue. TokenPositionDescriptor.sol is also in scope contract. As sponsor confirmed the issue with reasoning and judge reasoning for this issue confirmation too, This is absolutely a valid issue. Below is the additioanal context for raised escalations.

Issues found in used library dependencies has been judged as Medium at sherlock in past irrespective of new judging guidlines. Below are few issues for references,

1) Harpie audit, check Issue M-6: Signature malleability not protected against---Similar openzeppelin library bug issue judged as Medium, However the library issue was High severity and here it is critical severity.

2) UXD audit, check FullMath library overflow issue----library issue taken from uniswap

3) Bond audit, check Solmate code size issue-----solmate library token code size issue judged as Medium

4) I wouldn't had added this reference but i think it should be as the escalations focuses on rules and past issues. Check this issue. The issue is different but the escalation decision revolves around the used library where the Sherlock team reasoning for escalation rejection was

The library is used in the in-scope contract and the error impacts an in-scope contract. This is a valid issue.

To be noted, all these issues were happened before July, 2023 and the new rules made significant judging changes after July, 2023.(This was not intended to add but to respond a escalation, It was necessary to add for reference purpose only)

Per the sherlock rules too, This finding is valid and Sherlock rules states,

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.

Sherlock docs reference here

Contest readme.md also dont have this as known issue even the previous audits done by kyberSwap team does not highlight this critical issue too. I believe this issue is correctly judged by the lead judge and it's good that this issue is being found at sherlock which was missed in previous audits.

I hope, the above explanation is good enough for the validation of this issue.

Thank you!

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.

jkoppel commented 12 months ago

@jingyi2811 The part of the Sherlock rules you cite was added just 9 days ago. https://discord.com/channels/812037309376495636/1087792569196486706/1151162792074813592

nevillehuang commented 12 months ago

Escalate

i don't agree with both escalations.

UUPSUpgradeable vulnerability in OpenZeppelin Contracts is a critical vulnerability per the openzeppelin which was found in 2021 and to be noted this was the only second critical issue found till date in openzeppelin library. If the implementation is not initialized, someone can initialize and try to destroy the implementation which i agree with the sponsor comment here.

It should be noted here that TokenPositionDescriptor.sol as well as PoolOracle.sol is affected from this issue. TokenPositionDescriptor.sol is also in scope contract. As sponsor confirmed the issue with reasoning and judge reasoning for this issue confirmation too, This is absolutely a valid issue. Below is the additioanal context for raised escalations.

Issues found in used library dependencies has been judged as Medium at sherlock in past irrespective of new judging guidlines. Below are few issues for references,

Harpie audit, check Issue M-6: Signature malleability not protected against---Similar openzeppelin library bug issue judged as Medium, However the library issue was High severity and here it is critical severity.

UXD audit, check FullMath library overflow issue----library issue taken from uniswap

Bond audit, check Solmate code size issue-----solmate library token code size issue judged as Medium

I wouldn't had added this reference but i think it should be as the escalations focuses on rules and past issues. Check this issue. The issue is different but the escalation decision revolves around the used library where the Sherlock team reasoning for escalation rejection was

The library is used in the in-scope contract and the error impacts an in-scope contract. This is a valid issue.

To be noted, all these issues were happened before July, 2023 and the new rules made significant judging changes after July, 2023.(This was not intended to add but to respond a escalation, It was necessary to add for reference purpose only)

Per the sherlock rules too, This finding is valid and Sherlock rules states,

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.

Sherlock docs reference here

Contest readme.md also dont have this as known issue even the previous audits done by kyberSwap team does not highlight this critical issue too. I believe this issue is correctly judged by the lead judge and it's good that this issue is being found at sherlock which was missed in previous audits.

I hope, the above explanation is good enough for the validation of this issue.

Thank you!

Sure it is valid in sponsors eyes and yes it is not made a known issue. The point here is it falls under admin error, and thus is invalid according to sherlocks rule here. There is no existing risks, and if there is, it revolves around future deployment errors by the admin, where historically admin errors has not been accepted as valid issues.

On top of that, all the issues u quoted involves user facing functions, whereas this issue revolves admin facing functions. This is in addition to the fact that there will never be another initialize() function with OZ 4.3.1 for a user to call for either contracts. Will let @hrishibhat decide on this one.

jingyi2811 commented 12 months ago

If a suggestion made in the in-scope file fix the problem, I would say this is a valid issue. Kindly read 0x52 duplicate issue of this.

0xRizwan commented 12 months ago

Escalate i don't agree with both escalations. UUPSUpgradeable vulnerability in OpenZeppelin Contracts is a critical vulnerability per the openzeppelin which was found in 2021 and to be noted this was the only second critical issue found till date in openzeppelin library. If the implementation is not initialized, someone can initialize and try to destroy the implementation which i agree with the sponsor comment here. It should be noted here that TokenPositionDescriptor.sol as well as PoolOracle.sol is affected from this issue. TokenPositionDescriptor.sol is also in scope contract. As sponsor confirmed the issue with reasoning and judge reasoning for this issue confirmation too, This is absolutely a valid issue. Below is the additioanal context for raised escalations. Issues found in used library dependencies has been judged as Medium at sherlock in past irrespective of new judging guidlines. Below are few issues for references,

Harpie audit, check Issue M-6: Signature malleability not protected against---Similar openzeppelin library bug issue judged as Medium, However the library issue was High severity and here it is critical severity.

UXD audit, check FullMath library overflow issue----library issue taken from uniswap

Bond audit, check Solmate code size issue-----solmate library token code size issue judged as Medium

I wouldn't had added this reference but i think it should be as the escalations focuses on rules and past issues. Check this issue. The issue is different but the escalation decision revolves around the used library where the Sherlock team reasoning for escalation rejection was

The library is used in the in-scope contract and the error impacts an in-scope contract. This is a valid issue.

To be noted, all these issues were happened before July, 2023 and the new rules made significant judging changes after July, 2023.(This was not intended to add but to respond a escalation, It was necessary to add for reference purpose only) Per the sherlock rules too, This finding is valid and Sherlock rules states,

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.

Sherlock docs reference here Contest readme.md also dont have this as known issue even the previous audits done by kyberSwap team does not highlight this critical issue too. I believe this issue is correctly judged by the lead judge and it's good that this issue is being found at sherlock which was missed in previous audits. I hope, the above explanation is good enough for the validation of this issue. Thank you!

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.

I agree with @jingyi2811 escalation and the issue is correcty judged and it is valid.

@jkoppel Per the escalation, it is clearly explained the issues judement of such libraries issue have been judged as Medium in past irrespective of new juding guidlines. quoting the link shared by you here. sherlock team informs about the documentation updates. This announcement by sherlock team does not mention that the new rules wont be applicable to the contest happened before the rules announcement. Its a documentation update which ofcourse help in judging the contest. Please do check the point 4 in @jingyi2811 escalation.

@nevillehuang Thanks for agreeing on some points. Please note the issue arises from the openzeppelin library. Having a critical issue in dependency library is not an admin fault. This can not be considered as an admin error since the admin who is nothing but protocol team has been made aware of this critical issue via sherlock audit reports. If i am correct, the contracts were being audited two times in past and this issue was not raised in any audit report. The sponsor is confirming the issue with the reasoning and the lead judge is also confirming it. I believe the issue is valid. However, Let it be decided by sherlock team and lead judge.

jkoppel commented 12 months ago

@mohammedrizwann123 Can you give an explanation for why this issue should be judged differently from the Blueberry issue I cited?

Trumpero commented 11 months ago

I believe this issue should be considered a valid medium. It isn't obligatory for the admin to initialize the PoolOracle contract after deploying it if the correct version of OZ was utilized. It's a fact that KyberSwap didn't initialize their PoolOracle contracts until this bug was spotted recently before the contest, and they will move to OZ 4.3.2 for the future deployments. However, the protocol team didn't identify this issue as a known issue in the contest docs, so this issue should be valid according to Sherlock rules.

Additionally, this issue is truly in-scope since the PoolOracle contract inherits the UUPSUpgradeable contract version 4.3.1; it's not similar to using a library. The entire vulnerability exists in the PoolOracle contract, which can be exploited by an attacker.

jkoppel commented 11 months ago

It's a fact that KyberSwap didn't initialize their PoolOracle contracts until this bug was spotted recently before the contest

Link to transaction or discussion please?

I am still seeing nothing to differentiate this from the Blueberry issue I cited.

nevillehuang commented 11 months ago

This finding was reported by the OZ team in september of 2021 here:

https://forum.openzeppelin.com/t/security-advisory-initialize-uups-implementation-contracts/15301

Pretty sure its not “recent”, unless kyberswap intended to mistakenly leave the bug there for 2 years

hrishibhat commented 11 months ago

To address the @jkoppel's comments on blueberry the context of that issue does not apply here: That issue involved a Mockup contract and Sponsor had an unusual implementation mechanism that was already in place. That issue was discussed extensively with the Lead Watson and resolved. On the blueberry decision, there are strong arguments for both sides. But the factors of that decision do not apply here given the Mockup library.

As pointed out by the above escalation from @jingyi2811 historically using vulnerable external libraries is a valid issue. Especially for the external libraries that have a known bug in them. And the update of the library-related rule in the rulebook will not impact the decision here as they have always been rewarded. Also the update was intended to address cases like these. Will add more clarity on the same.

This is not an admin error. To simplify an admin error is using an incorrect parameter that causes loss because admin is expected to input the right parameter that is supposed to behave as expected. In this case, admin expects the the external library to work correctly.

Hence agree with the Lead Judge and escalations that consider this a valid issue.

hrishibhat commented 11 months ago

Result: Medium Has duplicates Considering this issue a valid medium based on the above comment.

sherlock-admin2 commented 11 months ago

Escalations have been resolved successfully!

Escalation status:

jingyi2811: accepted
nevillehuang: rejected
jkoppel: rejected

manhlx3006 commented 11 months ago

For current deployments, we have initialized all contracts with a multisig as the owner. PR to update dependencies for future deployments: https://github.com/KyberNetwork/ks-elastic-sc/pull/21

IAm0x52 commented 11 months ago

Fix looks good. Update to OZ library only changes UUPS and nothing else

sherlock-audit / 2023-07-kyber-swap-judging

MohammedRizwan - UUPSUpgradeable vulnerability in OpenZeppelin Contracts #25

UUPSUpgradeable vulnerability in OpenZeppelin Contracts

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation