WellUpgradeable.sol will become un-upgradeable if any of the underlying tokens is migrated to another address

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-08-basin/blob/7e08ff591df0a2ade7d5618113dda2621cd899bc/src/WellUpgradeable.sol#L79-L84

Vulnerability details

Impact

Contract will not be upgradeable, causing the protocol to miss out on the potential new features of the newly migrated ERC20 token in use beacause attempts to use the new token during well upgrade will fail, potentially forcing redeployment which defeats the purpose of the well's upgradability.

Proof of Concept

According to the information provided in the readme, the protocol will be working with all ERC20 tokens.

Scoping Q & A

//...

ERC20 used by the protocol Any (all possible ERC20s)

Lots of tokens currently in use in the DeFi space exist in versions in which the tokens are migrated from a previous version to another. This often involves, creating a new token contract with a different address and updating and introducing new characteristics and so on. This is slightly similar to token upgrades, but in this case, tokens are un-upgradeable and for some reason or the other, the devs decide to migrate the token to a new implementation/version. A recent example is MATIC's current migration to POLwhich will eventually become the new, dominant token address, while leaving the previous address redundant and unmanaged by the team. Note that it doesn't necessarily involve a name change.

Now, when the functions to upgrade the contract are called, upgradeTo and upgradeToAndCall, they both reroute to the _authorizeUpgrade which retrieves the tokens. The function also compares the tokens to each other ensuring that they're in the same order.

    function _authorizeUpgrade(address newImplementation) internal view override onlyOwner {
//...
        // verify the new well uses the same tokens in the same order.
        IERC20[] memory _tokens = tokens();
        IERC20[] memory newTokens = WellUpgradeable(newImplementation).tokens();
        require(_tokens.length == newTokens.length, "New well must use the same number of tokens");
        for (uint256 i; i < _tokens.length; ++i) {
            require(_tokens[i] == newTokens[i], "New well must use the same tokens in the same order");
        }
//...
    }

This means that upon underlying token migration, when its being fizzled out, or when new characteristics are introduced, attempts to upgrade the well using the new token's address will fail as the check highlighted above in the _authorizeUpgrade function will continue to revert with the error "New well must use the same tokens in the same order", not because the tokens aren't necessarily the same, but because the addresses are now different and do not match. As a result upgrading the well to a new implementation will be impossible, putting the protocol at an inherent disadvantage due to interactions with an older version of the token. In a worst case scenario, redeployment might be needed, which can mess with established protocol parameters and also defeats the purpose of the contract being upgradeable.

Tools Used

Manual code review

Recommended Mitigation Steps

Rather, i'd recommend removing the check for tokens being in the same order, as it also functions as a check for tokens being the same. This obviously places a lot of responsibility on the dev team, which I believe are trusted and competent to carefully handle contract upgrades. Otherwise, an owner protected function to change underying token addresses may be introduced, with logic ensuring that various paramteres of the tokens match.

Assessed type

Upgradable

[Brean0]

We disagree with the report as the current Well Upgradeable Implementation currently does not support the ability to change the underlying token of a Well. In order for a Well to change the underlying token, the Well Implementation would require additional functionality to remove the token in the Well and add a new token in the Well. Given there are cases where the new token aren't 1:1 with the original token (as seen in various token migrations in DeFi), those would require a new Well deployment due to the change in LP token supply that would occur.

Additionally, we do not feel like this removes the functionality for a potential token migration upgrade in the future. A developer can upgrade a Well to a Well Implementation that does support changing the underlying tokens (with the original tokens), and then upgrade the Well again with the new tokens.

Lastly, the original code did not have this check, and a report recommending this feature was added as the cost of error was much higher than the potential restriction that a developer might face with this requirement.

[alex-ppg]

The Warden attempts to outline a vulnerability in relation to the restrictions imposed on upgrades as a result of a submission in the previous Basin contest.

The restriction functions as expected, and the scenario of a token being migrated to another is not a "reasonable" scenario under the "all ERC20s" supported by the protocol. Even so, a token migration can be handled by upgrading the Well to an implementation supporting the exchange of the old token to the new one, and a new Well being deployed that supports the new token address (as the Sponsor correctly pointed out).

Based on the above, I cannot consider this a valid vulnerability.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid