A token that was deposited to CreateOfferer by Seaport may be stolen by malicious attacker.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/CreateOfferer.sol#L89

Vulnerability details

Impact

Deposited token of the CreateOfferer may be stolen by malicious attacker.

Proof of Concept

CreateOfferer.transferFrom() function is as follows.

File: CreateOfferer.sol
88:     //slither-disable-next-line erc20-interface
89:     function transferFrom(address from, address targetTokenReceiver, uint256 createOrderHashAsTokenId) external checkStage(Enums.Stage.transfer, Enums.Stage.ratify) {
90:         if (from != address(this)) revert Errors.FromNotCreateOfferer(from);
91:         transientState.receivers.targetTokenReceiver = targetTokenReceiver;
92:         IDelegateRegistry.DelegationType tokenType = RegistryHashes.decodeType(bytes32(createOrderHashAsTokenId));
...

This function is external and due to the lack of onlySeaport(msg.sender) modifier, it can be called by anybody. And that includes malicious attacker. Seaport will transfer some deposit token(ERC20, ERC721, ERC1155) into the CreateOfferer before it calls CreateOfferer.transferFrom() function. Attacker may monitor mempool against this and can hijack or front-run the normal CreateOfferer.transferFrom() call with his own malicious targetTokenReceiver address as parameter to get delegate token. As a result, the attacker will withdraw original deposit token with this stolen delegate token.

Tools Used

Manual Review

Recommended Mitigation Steps

CreateOfferer.transferFrom() function should be modified as follows.

File: CreateOfferer.sol
88:     //slither-disable-next-line erc20-interface
89:     function transferFrom(address from, address targetTokenReceiver, uint256 createOrderHashAsTokenId) external checkStage(Enums.Stage.transfer, Enums.Stage.ratify) onlySeaport(msg.sender) { // @audit <-- Added onlySeaport(msg.sender) modifier.
90:         if (from != address(this)) revert Errors.FromNotCreateOfferer(from);
91:         transientState.receivers.targetTokenReceiver = targetTokenReceiver;
92:         IDelegateRegistry.DelegationType tokenType = RegistryHashes.decodeType(bytes32(createOrderHashAsTokenId));
...

Assessed type

Access Control

[GalloDaSballo]

Flaggin but looks wrong

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/libraries/CreateOffererLib.sol#L378-L384

    function validateCreateOrderHash(address targetTokenReceiver, uint256 createOrderHash, bytes memory encodedOrder, IDelegateRegistry.DelegationType tokenType) internal view {
        uint256 actualCreateOrderHash = CreateOffererHelpers.calculateOrderHash(targetTokenReceiver, msg.sender, encodedOrder, tokenType);
        if (actualCreateOrderHash != createOrderHash) {
            revert CreateOffererErrors.CreateOrderHashInvariant(createOrderHash, actualCreateOrderHash);
        }
    }

[0xfoobar]

Incorrect, these are not real tokens only placeholders used to fulfill Seaport order matching. There is also access control and guards

[c4-sponsor]

0xfoobar (sponsor) disputed

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof