WellUpgradeable.sol will become un-upgradeable if any of the underlying tokens are upgraded.

[c4-bot-1]

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 implementations of the newly ungraded ERC20 toke in use.

Proof of Concept

According to the information provided in the readme, the protocol will be working with all ERC20 tokens, taking into consideration only upgradeability behaviour in scope.

Scoping Q & A

//...

ERC20 used by the protocol Any (all possible ERC20s)

//...

ERC20 token behaviors in scope

Question Answer

//...

Upgradeability In scope

Lots of tokens currently in use in the DeFi space are upgradeable, as it allows for introducing new characteristics, fix potential bugs etc, without having to go through the process of rewriting and redeploying the token.

Upgrading a token, however changes its proxy instance address. This is important as interactions with upgradeable tokens are not done with its original contract address, but rather the address of the proxy instance in use.

Taking USDC as our example, its address on Ethereum mainnet is 0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48. This will be the provided/required address when querying USDC address (see sites like coinmarketcap, coingecko, metamask etc).

This address however, is the address of its last proxy instance. Digging into the contract code, we'll discover that the actual toke address is 0x43506849d7c04f9138d1a2050bbf3a0c054402dd. Also, we can see that there had been an instance at address 0xa2327a938febf5fec13bacfb16ae10ecbc4cbdcf.

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 upgrade, when its being fizzled out, or when new characteristics are introduced, attempts to upgrade the well using the token's proxy 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 as a result 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

I'd would not workng with upgradeable tokens, but lots of tokens are upgradeable.
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

[c4-bot-1]

Withdrawn by ZanyBonzy