Griefing of `execute` transaction sender

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L154 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L92 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L108

Vulnerability details

Description

In function execute from BlurExchange contract there is a call of _executeTokenTransfer function. The last one contains the following logic:

function _executeTokenTransfer(
    address collection,
    address from,
    address to,
    uint256 tokenId,
    uint256 amount,
    AssetType assetType
) internal {
    /* Assert collection exists. */
    require(_exists(collection), "Collection does not exist");

    /* Call execution delegate. */
    if (assetType == AssetType.ERC721) {
        executionDelegate.transferERC721(collection, from, to, tokenId);
    } else if (assetType == AssetType.ERC1155) {
        executionDelegate.transferERC1155(collection, from, to, tokenId, amount);
    }
}

Both transferERC721 and transferERC1155 contains the following check:

require(revokedApproval[from] == false, "User has revoked approval");

So, there is a possibility for the sell.order.trader to revoke the approval while the execute is not included in a block. For example, this can be done using front-running.

Such an attack leads to the griefing of the execute transaction sender who spends value for the gas for the failed transaction.

Recommended Mitigation Steps

Add delay on revokeApproval logic. As an example, one of the possible ways is to use a check

require(lastRevokeApprovalTimestamp[from] == 0 || block.timestamp < lastRevokeApprovalTimestamp[from] + REVOKE_APPROVAL_DELAY, "User has revoked approval");

[Minh-Trng]

this seems like a somewhat artificial way of griefing. A griefer needs to put up an order and repeatedly revoke and unrevoke just so someone pays a bit of gas

[GalloDaSballo]

Any allowance has the risk of front-running

Any order signed can be executed

Preventing that would create further issues and risks

I think the finding comes from good place, but I don't think any vulnerability was shown, am closing as invalid