Protocol will fail for ERC1155 tokens

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/libraries/DelegateTokenTransferHelpers.sol#L23-L24

Vulnerability details

Issue

DelegateTokenTransferHelpers::checkERC1155BeforePull() and DelegateTokenTransferHelpers::pullERC1155AfterCheck() perform "set and check" operations on erc1155Pulled.flag which will always revert. In the first function, the value of erc1155Pulled.flag is set to ERC1155_PULLED, and in the second function, it reverts if the value of erc1155Pulled.flag is set to ERC1155_PULLED.

Code snippet from DelegateTokenTransferHelpers::checkERC1155BeforePull():

if (erc1155Pulled.flag == ERC1155_NOT_PULLED) {
  erc1155Pulled.flag = ERC1155_PULLED;
}

Code snippet from DelegateTokenTransferHelpers::pullERC1155AfterCheck():

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

The DelegateTokenTransferHelpers::checkAndPullByType() function calls the above two methods back to back, and hence the DelegateTokenTransferHelpers::checkAndPullByType() function call for ERC1155 is bound to revert every time.

Impact

DelegateTokenTransferHelpers::checkAndPullByType() is called from the DelegateToken::create() function. The DelegateToken::create() is the function where new Delegation and Principal tokens are created for the different kind of tokens. Because DelegateTokenTransferHelpers::checkAndPullByType() will revert, it will cause DelegateToken::create() to revert as well for ERC1155, thereby making the protocol not work for ERC1155.

Proof of Concept

Below is a proof of concept that can be run on Remix directly. The proof of concept below is the minimal setup extracted from the actual codebase to simulate the issue.

pragma solidity ^0.8.21;

contract TestContract {
    Structs.Uint256 internal erc1155PullAuthorization = Structs.Uint256(DelegateTokenTransferHelpers.ERC1155_NOT_PULLED);

    function runTest() external {
        DelegateTokenTransferHelpers.checkAndPullByType(
            erc1155PullAuthorization
        );
    }
}

library Structs {
    struct Uint256 {
        uint256 flag;
    }
}

library DelegateTokenTransferHelpers {
    uint256 internal constant ERC1155_NOT_PULLED = 5;
    uint256 internal constant ERC1155_PULLED = 6;

    error ERC1155NotPulled();

    function checkAndPullByType(
        Structs.Uint256 storage erc1155Pulled
    ) external {
        checkERC1155BeforePull(erc1155Pulled);
        pullERC1155AfterCheck(
            erc1155Pulled
        );
    }

    function checkERC1155BeforePull(
        Structs.Uint256 storage erc1155Pulled
    ) internal {
        if (erc1155Pulled.flag == ERC1155_NOT_PULLED) {
            erc1155Pulled.flag = ERC1155_PULLED;
        }
    }

    function pullERC1155AfterCheck(
        Structs.Uint256 storage erc1155Pulled
    ) internal {
        if (erc1155Pulled.flag == ERC1155_PULLED) {
            revert ERC1155NotPulled();
        }
    }
}

Tools Used

Manual analysis

Recommended Mitigation Steps

Update the the functions DelegateTokenTransferHelpers::checkERC1155BeforePull() and DelegateTokenTransferHelpers::pullERC1155AfterCheck() so that the first function doesn't set values which make the second function revert.

Assessed type

Other

[c4-judge]

GalloDaSballo marked the issue as duplicate of #84

[c4-judge]

GalloDaSballo changed the severity to 3 (High Risk)

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid