WellUpgradeable#_authorizeUpgrade should check tokens in the new implementation

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/WellUpgradeable.sol#L65-L85

Vulnerability details

Impact

The current composition of contracts and proxies is as follows:

(entry point) (delegateCalls) (minimalProxy) (delegateCalls and append immutableCallArgs) (original implementation)
ERC1967Proxy -> WellUpgradeable -> WellUpgradeable

If we want to upgrade the implementation of the ERC1967Proxy, we should deploy the new WellUpgradeable and bore it from the Aquifier. The problem is that we may bore it by providing different tokens data when the new implementation is being cloned into minimalProxy.(NOTE There is a problem is we have same tokens, but their order is swapped) The next step is to pass the minimal proxy address to the ERC1967Proxy#upgradeTo function, which will only check if the implementation is "authorized" by the aquifer address saved in the current minimal proxy and if the new implementation is valid ERC1967Proxy implementation. If so, WellUpgradeable minimal proxy is updated for the ERC1967Proxy. If the tokens inside the new minimal proxy are different from the last, this may lead to a lot of problems, such as the inability for users to claim their original tokens liquidity.

Another realistic scenario is to have the same tokens set, but in reversed order. This will also cause problems, because of the way Well reads from storage and lods I and J:

Proof of Concept

Example:

• Imagine we have a WellUpgradable with tokens set to [BEAN;USDC].
  When we add liquidity, the contract will store corresponding reserves for the tokens in two consequent storage slots (RESERVES_STORAGE_SLOT -> BEAN Reserves; RESERVES_STORAGE_SLOT + 32 = USDC Reserves).

• If the implementation is updated and the new minimal proxy received [USDC;BEAN] for tokens, this will result in the following miscalculations:

• If we try to swap from BEAN -> USDC

function _swapFrom(
        IERC20 fromToken,
        IERC20 toToken,
        uint256 amountIn,
        uint256 minAmountOut,
        address recipient
    ) internal returns (uint256 amountOut) {
        IERC20[] memory _tokens = tokens();
        (uint256 i, uint256 j) = _getIJ(_tokens, fromToken, toToken);

        uint256[] memory reserves = _updatePumps(_tokens.length);

        reserves[i] += amountIn;
        uint256 reserveJBefore = reserves[j];
        reserves[j] = _calcReserve(wellFunction(), reserves, j, totalSupply());

• i will be 1 and j will be 0
• _updatePumps calls _getReserves, which will read the reserves from ERC1967 storage (reserves[0] = bean reserves; reserves[1] = usdc reserves)
• reserves[I] + amountIn corresponds to increasing the value of the slot responsible for usdc reserves, instead of the one, which holds bean
• The following will result in unexpected price and liquidity calculations and may be unnoticed for some period

Tools Used

Manual Review

Recommended Mitigation Steps

Inise _authorizeUpgrade check if newImplmentation tokens are the same and in the same order as the current minimal proxy.

Assessed type

Invalid Validation

[alex-ppg]

The Warden outlines an issue with the upgrade authorization methodology in relation to the tokens supported by the WellUpgradeable implementation.

This particular submission is slightly difficult to properly assess fairly. A well's upgrade is meant to be an authorized action, meaning that a user upgrades the well they deployed. Attempting to upgrade a well with a token configuration of USDC and DAI to a new well that uses different tokens is a user mistake and thus cannot be considered an HM vulnerability per the relevant SC verdict.

The main problem here is that upgrades are unrestricted entirely as demonstrated by the #52 issue group. Based on this fact as well as the ability to capitalize on an improper token configuration via swaps (i.e. reversing the reserve calculations), I am inclined to accept this submission as a valid medium-risk issue.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as selected for report

[Honour-d-dev]

Hi @alex-ppg excellent judging.

However, i believe this issue is valid but should be a low severity. The reason is based on the fact that upgrades are (or should be) an Admin task as explained by #52, so the issue of "wrong tokens" or "wrong order of tokens" would be an admin mistake .
If Admin is a trusted (as it should be assumed) , then this issue does prevent the admin from making this mistake but it doesn't meet the H/M vulnerability criteria.

[NicolaMirchev]

Hi @alex-ppg excellent judging.

However, i believe this issue is valid but should be a low severity. The reason is based on the fact that upgrades are (or should be) an Admin task as explained by #52, so the issue of "wrong tokens" or "wrong order of tokens" would be an admin mistake . If Admin is a trusted (as it should be assumed) , then this issue does prevent the admin from making this mistake but it doesn't meet the H/M vulnerability criteria.

Hey,

Here are my arguments about why this should be judged as a valid Medium issue:

• Regarding the trust assumptions: There are no trusted roles in the context for this contest
  
• The following means that even if WellUpgradeable can be upgraded by anyone #52 is fixed, the issue of reordered token params is still valid when this function is called, which will result in major impact and unexpected behavior.

[Honour-d-dev]

Hi,

By Admin i'm referring to the protocol itself ,so i believe trust is automatically implied. no?
As the protocol cannot intentionally endanger users funds

[NicolaMirchev]

The contest page is the first source of truth, and it is specifically stated that there are no trusted roles, meaning WellUpdatable owners are not trusted as protocol admins. This, combined with the judge's statement that the misconfiguration could be capitalized, supports the decision to categorize the issue as Medium severity.

[Honour-d-dev]

Hi,

I don't think this is true

WellUpdatable owners are not trusted

Obviously the sponsors can shed some light on it, but If wellUpgradeable owners are untrusted then "wrong tokens" or "wrong order tokens" would be the least of the problems for anyone using such a pool. Owner can upgrade to entirely different logic while maintaining same tokens and token order and this cannot be prevented because the Aquifer is permissionless.

If this is the case then i strongly advise the team against this.

[alex-ppg]

Hey @Honour-d-dev and @NicolaMirchev, thank you for your PJQA contributions.

This particular issue as originally stated is tricky to judge due to the absence of trusted roles in the protocol. I am inclined to agree, however, that a malicious upgrade could result in significantly more problems than simply a token swap.

The vulnerability is incidentally applicable due to upgrades requiring no privilege whatsoever as a result of #52, so I will proceed with grouping this issue there as a lower severity duplicate.

To note, the Impact chapter of this exhibit does outline that the validation during an upgrade performed is weak which is why I consider it eligible as a duplicate, however, the medium-risk rating that was assigned to it showcases that the submitter was not fully aware of the implications of this weak validation. As such, a partial reward will be assigned per relevant C4 guidelines (i.e. same root cause being weak validation of upgrades but severity categorization being incorrect).

[c4-judge]

alex-ppg marked the issue as duplicate of #52

[c4-judge]

alex-ppg marked the issue as partial-50

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)

[NicolaMirchev]

Hey @Honour-d-dev and @NicolaMirchev, thank you for your PJQA contributions.

This particular issue as originally stated is tricky to judge due to the absence of trusted roles in the protocol. I am inclined to agree, however, that a malicious upgrade could result in significantly more problems than simply a token swap.

The vulnerability is incidentally applicable due to upgrades requiring no privilege whatsoever as a result of #52, so I will proceed with grouping this issue there as a lower severity duplicate.

To note, the Impact chapter of this exhibit does outline that the validation during an upgrade performed is weak which is why I consider it eligible as a duplicate, however, the medium-risk rating that was assigned to it showcases that the submitter was not fully aware of the implications of this weak validation. As such, a partial reward will be assigned per relevant C4 guidelines (i.e. same root cause being weak validation of upgrades but severity categorization being incorrect).
Hey, @alex-ppg

We have the original issue submitted.
Please consider closing this one if you judge it as a duplicate.

[c4-judge]

alex-ppg marked the issue as not selected for report