Delegate - sces60107's results

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L89 https://github.com/code-423n4/2023-09-delegate/blob/main/src/libraries/DelegateTokenTransferHelpers.sol#L79 https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L408

Vulnerability details

Impact

The initial value of erc1155PullAuthorization is ERC1155_NOT_PULLED. And it would be updated to ERC1155_PULLED in DelegateTokenTransferHelpers.checkERC1155BeforePull. And when DelegateToken receives the erc 1155 token, DelegateToken.onERC1155Received will be trigger then update erc1155PullAuthorization to ERC1155_NOT_PULLED. However, DelegateToken.onERC1155Received doesn’t check if the msg.sender is the erc1155 contract. Any contract can call the function to update erc1155PullAuthorization. Therefore, the real token transfer would be blocked.

Proof of Concept

Usually, DelegateToken.onERC1155Received would be called by ERC115 safeTransferFrom. https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L89

    /// @inheritdoc IERC1155Receiver
    function onERC1155Received(address operator, address, uint256, uint256, bytes calldata) external returns (bytes4) {
        TransferHelpers.revertInvalidERC1155PullCheck(erc1155PullAuthorization, operator);
        return IERC1155Receiver.onERC1155Received.selector;
    }

And it calls revertInvalidERC1155PullCheck to update erc1155PullAuthorization to ERC1155_NOT_PULLED. Also, we can notice that onERC1155Received reverts when erc1155Pulled is not ERC1155_PULLED. https://github.com/code-423n4/2023-09-delegate/blob/main/src/libraries/DelegateTokenTransferHelpers.sol#L79

    function checkERC1155Pulled(Structs.Uint256 storage erc1155Pulled, address operator) internal returns (bool) {
        if (erc1155Pulled.flag == ERC1155_PULLED && address(this) == operator) {
            erc1155Pulled.flag = ERC1155_NOT_PULLED;
            return true;
        }
        return false;
    }

    function revertInvalidERC1155PullCheck(Structs.Uint256 storage erc1155PullAuthorization, address operator) internal {
        if (!checkERC1155Pulled(erc1155PullAuthorization, operator)) revert Errors.ERC1155PullNotRequested(operator);
    }

Let's take a look at the flash loan logic for ERC1155. https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L408

    function flashloan(Structs.FlashInfo calldata info) external payable nonReentrant {
        …
        } else if (info.tokenType == IDelegateRegistry.DelegationType.ERC1155) {
            RegistryHelpers.revertERC1155FlashAmountUnavailable(delegateRegistry, info);
            TransferHelpers.checkERC1155BeforePull(erc1155PullAuthorization, info.amount);
            IERC1155(info.tokenContract).safeTransferFrom(address(this), info.receiver, info.tokenId, info.amount, "");
            Helpers.revertOnCallingInvalidFlashloan(info);
            TransferHelpers.pullERC1155AfterCheck(erc1155PullAuthorization, info.amount, info.tokenContract, info.tokenId);
        }
    }

The normal case is:

• TransferHelpers.checkERC1155BeforePull updates erc1155PullAuthorization to ERC1155_PULLED
• TransferHelpers.pullERC1155AfterCheck calls IERC1155(underlyingContract).safeTransferFrom to transfer back the erc1155 token
• DelegateToken.onERC1155Received would be trigger to update erc1155PullAuthorization to ERC1155_NOT_PULLED

But the following case could happen and make the translation revert.

• TransferHelpers.checkERC1155BeforePull updates erc1155PullAuthorization to ERC1155_PULLED
• Helpers.revertOnCallingInvalidFlashloan(info) is called. The flash loan caller may call some smart contracts.
• One of the contract could be malicious and it directly calls DelegateToken.onERC1155Received to update erc1155PullAuthorization to ERC1155_NOT_PULLED
• TransferHelpers.pullERC1155AfterCheckcallsIERC1155(underlyingContract).safeTransferFrom` to transfer back the erc1155 token
• DelegateToken.onERC1155Received would be trigger but it would revert since erc1155PullAuthorization is not ERC1155_PULLED

Tools Used

Manual Review

Recommended Mitigation Steps

The root cause is that any contract can call DelegateToken.onERC1155Received. The only legit caller is the erc 1155 token contract. Add a state variable to record the erc 1155 token contract.

    Structs.Uint256 internal erc1155PullAuthorization = Structs.Uint256(TransferHelpers.ERC1155_NOT_PULLED);
+   address internal erc1155contract = address(0);

    /// @inheritdoc IERC1155Receiver
    function onERC1155Received(address operator, address, uint256, uint256, bytes calldata) external returns (bytes4) {
+       if (msg.sender != erc1155contract) revert Errors.InvalidERC1155ContractAddress();
        TransferHelpers.revertInvalidERC1155PullCheck(erc1155PullAuthorization, operator);
        return IERC1155Receiver.onERC1155Received.selector;
    }



    function flashloan(Structs.FlashInfo calldata info) external payable nonReentrant {
        …
        } else if (info.tokenType == IDelegateRegistry.DelegationType.ERC1155) {
            RegistryHelpers.revertERC1155FlashAmountUnavailable(delegateRegistry, info);
            TransferHelpers.checkERC1155BeforePull(erc1155PullAuthorization, info.amount);
+           erc1155contract  = info.tokenContract;
            IERC1155(info.tokenContract).safeTransferFrom(address(this), info.receiver, info.tokenId, info.amount, "");
            Helpers.revertOnCallingInvalidFlashloan(info);
            TransferHelpers.pullERC1155AfterCheck(erc1155PullAuthorization, info.amount, info.tokenContract, info.tokenId);
+           erc1155contract  = address(0);
        }
    }

Assessed type

Token-Transfer