Lack of revert upon undefined delegation type in many places may change global state without doing anything

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L385 https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L321 https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L209

Vulnerability details

Impact

The contract DelegateToken does work with three different types for his delegation tokens, namely ERC20, ERC721 and ERC1155. For other tokens, it is expected to revert as seen in CreateOfferer#L164. However, in the links above, it does not revert. This is bad because, for example, in DelegateToken::withdraw, it will modify global balances and registry values and continue with its execution even if the token type is not supported, so no withdraw will be made, thus corrupting the global state of the contract.

Proof of Concept

NOTE -> I will work with DelegateToken::withdraw as an example, the links above are pretty similar so the explanation works for all of them

    function withdraw(uint256 delegateTokenId) external nonReentrant {

        // HERE modify global state

        bytes32 registryHash = StorageHelpers.readRegistryHash(delegateTokenInfo, delegateTokenId);
        StorageHelpers.writeRegistryHash(delegateTokenInfo, delegateTokenId, bytes32(StorageHelpers.ID_USED));
        // Sets registry pointer to used flag
        StorageHelpers.revertNotMinted(registryHash, delegateTokenId);
        (address delegateTokenHolder, address underlyingContract) = RegistryHelpers.loadTokenHolderAndContract(delegateRegistry, registryHash);
        StorageHelpers.revertInvalidWithdrawalConditions(delegateTokenInfo, accountOperator, delegateTokenId, delegateTokenHolder);
        StorageHelpers.decrementBalance(balances, delegateTokenHolder);
        delete delegateTokenInfo[delegateTokenId][StorageHelpers.PACKED_INFO_POSITION]; // Deletes both expiry and approved
        emit Transfer(delegateTokenHolder, address(0), delegateTokenId);
        IDelegateRegistry.DelegationType delegationType = RegistryHashes.decodeType(registryHash);
        bytes32 underlyingRights = RegistryHelpers.loadRights(delegateRegistry, registryHash);

        // HERE check for supported tokens, reTURN if it is not

        if (delegationType == IDelegateRegistry.DelegationType.ERC721) {
            uint256 erc721UnderlyingTokenId = RegistryHelpers.loadTokenId(delegateRegistry, registryHash);
            RegistryHelpers.revokeERC721(delegateRegistry, registryHash, delegateTokenHolder, underlyingContract, erc721UnderlyingTokenId, underlyingRights);
            StorageHelpers.burnPrincipal(principalToken, principalBurnAuthorization, delegateTokenId);
            IERC721(underlyingContract).transferFrom(address(this), msg.sender, erc721UnderlyingTokenId);
        } else if (delegationType == IDelegateRegistry.DelegationType.ERC20) {
            uint256 erc20UnderlyingAmount = StorageHelpers.readUnderlyingAmount(delegateTokenInfo, delegateTokenId);
            StorageHelpers.writeUnderlyingAmount(delegateTokenInfo, delegateTokenId, 0); // Deletes amount
            RegistryHelpers.decrementERC20(delegateRegistry, registryHash, delegateTokenHolder, underlyingContract, erc20UnderlyingAmount, underlyingRights);
            StorageHelpers.burnPrincipal(principalToken, principalBurnAuthorization, delegateTokenId);
            SafeERC20.safeTransfer(IERC20(underlyingContract), msg.sender, erc20UnderlyingAmount);
        } else if (delegationType == IDelegateRegistry.DelegationType.ERC1155) {
            uint256 erc1155UnderlyingAmount = StorageHelpers.readUnderlyingAmount(delegateTokenInfo, delegateTokenId);
            StorageHelpers.writeUnderlyingAmount(delegateTokenInfo, delegateTokenId, 0); // Deletes amount
            uint256 erc11551UnderlyingTokenId = RegistryHelpers.loadTokenId(delegateRegistry, registryHash);
            RegistryHelpers.decrementERC1155(
                delegateRegistry, registryHash, delegateTokenHolder, underlyingContract, erc11551UnderlyingTokenId, erc1155UnderlyingAmount, underlyingRights
            );
            StorageHelpers.burnPrincipal(principalToken, principalBurnAuthorization, delegateTokenId);
            IERC1155(underlyingContract).safeTransferFrom(address(this), msg.sender, erc11551UnderlyingTokenId, erc1155UnderlyingAmount, "");
        }
    }   ^================================================================= HERE there is no revert if type not supported

We see that global changes are made even if the token is not supported. As I do not have more time to dig inside the low-level changes made and because there is a precedence in CreateOfferer#L164, so it is something the developers thought about but did not implement in the DelegateToken contract, I consider it as a medium (it may be a high, though, it's up to the judges)

Tools Used

Manual analysis

Recommended Mitigation Steps

Just revert if the token is not supported to avoid making changes in global state as seen in CreateOfferer#L164

Assessed type

Error

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

Interesting discussion around inconsistent logic, I believe impact is limited (informational)

[c4-sponsor]

0xfoobar marked the issue as disagree with severity

[c4-sponsor]

0xfoobar (sponsor) disputed

[c4-sponsor]

0xfoobar marked the issue as agree with severity

[0xfoobar]

TransferHelpers.checkAndPullByType(erc1155PullAuthorization, delegateInfo) performs the typechecking

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof

[c4-judge]

GalloDaSballo removed the grade

[GalloDaSballo]

NC

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-c