Well.sol could not be updated

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/main/src/Aquifer.sol#L40-L51 https://github.com/code-423n4/2023-07-basin/blob/main/src/Well.sol#L22

Vulnerability details

Impact

Aquifer.sol is used by Clones.cloneDeterministic to create a new Well contract. However, Well seems to be an upgradeable contract. But Clone is not compatible with updatable contracts.

https://docs.openzeppelin.com/contracts/3.x/api/proxy

Proof of Concept

The answer in this post explains why Clone is not compatible with updatable contracts. https://forum.openzeppelin.com/t/contract-factory-for-upgradeable-erc721/11153

The problem is that the implementation address for the transparent proxy is stored in the transparent proxy's storage, but when the user passes through the clone, it will use the clone's storage, so it doesn't know where to delegate.

Tools Used

Manual review

Recommended Mitigation Steps

Do not use an updatable contract as an implementation address in Clone. If you need an update mechanism for Well, consider using Beacon.

Assessed type

Context

[c4-pre-sort]

141345 marked the issue as primary issue

[141345]

situation seems not exactly the same as the OZ link, need further look

[publiuss]

Wells should be considered malicious until proven innocent. Any Well that improperly clones a proxy contract such that it doesn't have a verifiable implementation should be considered malicious. A Well that is upgradable could be verified through the use of wellData. Therefore, this should not be considered a valid issue.

[Edited]

[c4-sponsor]

publiuss marked the issue as sponsor disputed

[alcueca]

Wells should be considered innocent until proven malicious

I think @publiuss meant that "Wells should be considered malicious until proven innocent".

That by itself would reduce the severity to High to Medium if it would just mean that the Well cannot be deployed through the Aquifer. It would still be High if the Well can be deployed but an upgrade renders it unusable.

Unfortunately, the warden hasn't provided a PoC of what is that it actually happens. Given that the tests for the Well use the Aquifer for deployment, we have proof that the Wells can be deployed through the Aquifer.

@publiuss, I can't see tests for Well upgrades, could you verify if those work?

[publiuss]

@alcueca I think @publiuss meant that "Wells should be considered malicious until proven innocent".

• You are correct thank you.

I can't see tests for Well upgrades, could you verify if those work?

• The Well contract provided is not intended to be upgradable. Thus, there are no tests for Well upgrades. In the future, someone could create a Well implementation that is upgradeable.

[alcueca]

The Well.sol provided inherits from ERC20PermitUpgradable and ReentrancyGuardUpgradable. Why didn't you inherit from ERC20Permit and ReentrancyGuard, then?

[publiuss]

The Well.sol provided inherits from ERC20PermitUpgradable and ReentrancyGuardUpgradable. Why didn't you inherit from ERC20Permit and ReentrancyGuard, then?

The intention is to have new Wells deployed via the clone library. It is not possible to provide constructor arguments through the clone function, so in order to have different deployments be initialized with different constructor arguments, an init function was used instead.

[alcueca]

Fair enough, I would recommend adding this to the documentation. I'm going to accept this finding as a grade-b QA on those grounds.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-b