Broken ERC1155 functions in DelegateTokenTransferHelper library

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/libraries/DelegateTokenTransferHelpers.sol#L72

Vulnerability details

Impact

The condition in DelegateTokenTransferHelpers.sol#L72 is incorrect:

        if (erc1155Pulled.flag == ERC1155_PULLED) {
            revert Errors.ERC1155NotPulled();
        }

as the flag is set to ERC1155_PULLED after calling checkERC1155BeforePull, either by calling checkAndPullByType or in the call path checkERC1155BeforePull->pullERC1155AfterCheck. This will make any attempt to pull ERC1155 tokens to revert (e.g.DelegateToken::flashloan for ERC1155 tokens or DelegateToken::create). I consider the severity to be a high because it could have gone into production without the devs noticing that such functionality is fundamentally broken. Moreover, there is no unit test covering the execution branch of the flashloan function for ERC1155 tokens, so the odds of deploying the code as it is is right now is pretty high, so the severity.

Proof of Concept

    // DelegateTokenTransferHelpers#L15-28
    function checkAndPullByType(Structs.Uint256 storage erc1155Pulled, Structs.DelegateInfo calldata delegateInfo) internal {
        ...

        <===================== HERE first it calls checkERC1155BeforePull and then pullERC1155AfterCheck
        } else if (delegateInfo.tokenType == IDelegateRegistry.DelegationType.ERC1155) {
            checkERC1155BeforePull(erc1155Pulled, delegateInfo.amount);
            pullERC1155AfterCheck(erc1155Pulled, delegateInfo.amount, delegateInfo.tokenContract, delegateInfo.tokenId);
        } else {

        ...
    }

    // DelegateTokenTransferHelpers#61-68
    function checkERC1155BeforePull(Structs.Uint256 storage erc1155Pulled, uint256 pullAmount) internal {
        if (pullAmount == 0) revert Errors.WrongAmountForType(IDelegateRegistry.DelegationType.ERC1155, pullAmount);
        if (erc1155Pulled.flag == ERC1155_NOT_PULLED) {
            erc1155Pulled.flag = ERC1155_PULLED; <================================ HERE flag set to ERC1155_PULLED
        } else {
            revert Errors.ERC1155Pulled();
        }
    }

    // DelegateTokenTransferHelpers#L77-83
    function pullERC1155AfterCheck(Structs.Uint256 storage erc1155Pulled, uint256 pullAmount, address underlyingContract, uint256 underlyingTokenId) internal {
        IERC1155(underlyingContract).safeTransferFrom(msg.sender, address(this), underlyingTokenId, pullAmount, "");
        if (erc1155Pulled.flag == ERC1155_PULLED) { <======================== HERE check if it is pulled and revert, which is always the execution flow reaches this point
            revert Errors.ERC1155NotPulled();
        }
    }

For DelegateToken::flashloan, we see that it calls checkERC1155BeforePull and a few lines after it calls pullERC1155AfterCheck, so it will always revert and the flash-loan functionality will be broken for ERC1155 tokens.

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the condition in pullERC1155AfterCheck to != so that it reverts if the token was NOT pulled or set the flag to ERC1155_PULLED inside pullERC1155AfterCheck instead of checkERC1155BeforePull

Assessed type

Other

[c4-judge]

GalloDaSballo marked the issue as duplicate of #171

[c4-judge]

GalloDaSballo marked the issue as duplicate of #84

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid