search

code-423n4 / 2023-09-delegate-findings

2 stars 1 forks source link

Any contract can call `DelegateToken.onERC1155Received` to update `erc1155PullAuthorization` and block the transfer. #354

Open c4-submissions opened 10 months ago

c4-submissions commented 10 months ago

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:

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

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

0xfoobar commented 9 months ago

Need to double-check this but issue is plausible

c4-sponsor commented 9 months ago

0xfoobar (sponsor) confirmed

GalloDaSballo commented 9 months ago

There's 2 sides to this finding: 1) Impact -> Impact doesn't seem to be fully explored, this similar idea of anyone being able to call the onReceived is fairly widespread but the finding simply uses it to cause a revert which is arguably just a grief

2) The lack of validation for the caller may be a component of a broader attack, since there's no verification that the intended ERC1155 is the one confirming the transfer

GalloDaSballo commented 9 months ago

It is possible for an external contract to prevent the Flashloan to work

However, the control as to how that would happen is under the callers control

While there is a lack of check for toggling of erc1155Pulled.flag this will cause the tx to revert

Due to the lowered impact, I'm downgrading to Qa

c4-judge commented 9 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)

c4-judge commented 9 months ago

GalloDaSballo marked the issue as grade-b

GalloDaSballo commented 9 months ago

Awarding QA - B in spite of low score due to the finding being notable

DadeKuma commented 9 months ago

I think that this issue is invalid.

onERC1155Received will not revert only when the current state is ERC1155_PULLED

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

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

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;
}

State ERC1155_PULLED is set here:

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

checkERC1155BeforePull is called only during a flashloan.

    function flashloan(Structs.FlashInfo calldata info) external payable nonReentrant {
        StorageHelpers.revertNotOperator(accountOperator, info.delegateHolder);
        if (info.tokenType == IDelegateRegistry.DelegationType.ERC721) {
            RegistryHelpers.revertERC721FlashUnavailable(delegateRegistry, info);
            IERC721(info.tokenContract).transferFrom(address(this), info.receiver, info.tokenId);
            Helpers.revertOnCallingInvalidFlashloan(info);
            TransferHelpers.checkERC721BeforePull(info.amount, info.tokenContract, info.tokenId);
            TransferHelpers.pullERC721AfterCheck(info.tokenContract, info.tokenId);
        } else if (info.tokenType == IDelegateRegistry.DelegationType.ERC20) {
            RegistryHelpers.revertERC20FlashAmountUnavailable(delegateRegistry, info);
            SafeERC20.safeTransfer(IERC20(info.tokenContract), info.receiver, info.amount);
            Helpers.revertOnCallingInvalidFlashloan(info);
            TransferHelpers.checkERC20BeforePull(info.amount, info.tokenContract, info.tokenId);
            TransferHelpers.pullERC20AfterCheck(info.tokenContract, info.amount);
        } 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);
        }
    }

This means that the info.receiver must call a malicious contract after it receives the token, which then should send an ERC1155 to DelegateToken, just to make the transaction revert without gaining anything?

What if the malicious contract is the 1155 contract itself? Then the mitigation would not have any effect, as it wouldn't revert when DelegateToken receives the token. I don't think this is a valid issue, as it's not preventable + the impact is non-existent.

GalloDaSballo commented 9 months ago

The finding highlights how the onERC1155Received pattern, without verifying the caller can be griefed

The impact falls under self-rekt / gotcha as the contract that is interacted with must be malicious

Interestingly enough this could be used to prevent claims from flashloans if the token is ERC1155

I believe the finding to be notable enough to warrant it being added to the report