c4-submissions commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L296-L322

Vulnerability details

Impact

ERC1155 tokens can never be used.

Proof of Concept

If we look at the create function in the delegateToken contract we can see the line : TransferHelpers.checkAndPullByType(erc1155PullAuthorization, delegateInfo); this line calls the TransferHelpers.checkAndPullByType so if we look into it we can see :

function checkAndPullByType(Structs.Uint256 storage erc1155Pulled, Structs.DelegateInfo calldata delegateInfo) internal {
        if (delegateInfo.tokenType == IDelegateRegistry.DelegationType.ERC721) {
            checkERC721BeforePull(delegateInfo.amount, delegateInfo.tokenContract, delegateInfo.tokenId);
            pullERC721AfterCheck(delegateInfo.tokenContract, delegateInfo.tokenId);
        } else if (delegateInfo.tokenType == IDelegateRegistry.DelegationType.ERC20) {
            checkERC20BeforePull(delegateInfo.amount, delegateInfo.tokenContract, delegateInfo.tokenId);
            pullERC20AfterCheck(delegateInfo.tokenContract, delegateInfo.amount);
        } else if (delegateInfo.tokenType == IDelegateRegistry.DelegationType.ERC1155) {
            checkERC1155BeforePull(erc1155Pulled, delegateInfo.amount);
            pullERC1155AfterCheck(erc1155Pulled, delegateInfo.amount, delegateInfo.tokenContract, delegateInfo.tokenId);
        } else {
            revert Errors.InvalidTokenType(delegateInfo.tokenType);
        }
    }

so when it is ERC1155, let us look at the checkERC1155BeforePull(erc1155Pulled, delegateInfo.amount):

        if (pullAmount == 0) revert Errors.WrongAmountForType(IDelegateRegistry.DelegationType.ERC1155, pullAmount);
        if (erc1155Pulled.flag == ERC1155_NOT_PULLED) {
            erc1155Pulled.flag = ERC1155_PULLED;
        } else {
            revert Errors.ERC1155Pulled();
        }
    }

we can see that Structs.Uint256 storage erc1155Pulled this is storage and if (erc1155Pulled.flag == ERC1155_NOT_PULLED) { erc1155Pulled.flag = ERC1155_PULLED; it is set to ERC1155_PULLED and now back at the checkAndPullByType the pullERC1155AfterCheck(erc1155Pulled, delegateInfo.amount, delegateInfo.tokenContract, delegateInfo.tokenId); is called. If we look into it :

 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) {
            revert Errors.ERC1155NotPulled();
        }
    }

now this if (erc1155Pulled.flag == ERC1155_PULLED) will revert because it is true and we know that in the checkERC1155BeforePull function it was storage and was set to already set to ERC1155_PULLED.

Tools Used

manual review.

Recommended Mitigation Steps

set to ERC1155_PULLED only after the actual pull and remove in the checkERC1155BeforePull function.

Assessed type

DoS

GalloDaSballo commented 10 months ago

This report should have had a coded POC

c4-judge commented 10 months ago

GalloDaSballo marked the issue as duplicate of #84

c4-judge commented 9 months ago

GalloDaSballo marked the issue as satisfactory