Blur Exchange contest - Trust's results

Dup of https://github.com/code-423n4/2022-10-blur-findings/issues/666

Dup of https://github.com/code-423n4/2022-10-blur-findings/issues/713

R

I believe this report introduces two significant centralization threats not discussed in referenced dup (esp. description point 2).

I will make a fact-based case of why this is a Med finding.

Firstly, it is not a dup of #713. #713 discusses a new executionDelegate which won't have revokedApproval set as previous one. That's not an actual issue because in Blur funds are approved to the executionDelegate, so a new delegate will not have users' approvals to begin with.

This report describes 2 centralization risks. Risk 1: Existing executionDelegate accepts any transferERC721 / transferERC20 / transferERC1155 calls from Exchange proxy address. Since there is no timelock (or a provable one), there is nothing stopping malicious or hacked owner from immediately upgrading the Exchange to this contract:

function _stealTokens(
    address token,
    address from,
    address to,
    uint256 tokenId,
    uint256 amount,
    AssetType assetType
) external onlyOwner {
    /* Call execution delegate. */
    if (assetType == AssetType.ERC721) {
        executionDelegate.transferERC721(token, from, to, tokenId);
    } else if (assetType == AssetType.ERC1155) {
        executionDelegate.transferERC1155(token, from, to, tokenId, amount);
    } else if (assetType == AssetType.ERC20) {
        executionDelegate.transferERC20(token, from, to, amount);
}

I will reiterate that this risk is not speculation on future code, it is a demonstration that existing code + dishonest owner results in rug pull of all Exchange funds. The existing code allows adding arbitrary abuse of approval, which is overly dangerous.

Risk 2: This risk shows admin can do a complete rug pull without any contract upgrade. Contracts are approved here:

function approveContract(address _contract) onlyOwner external {
        contracts[_contract] = true;
        emit ApproveContract(_contract);
    }

Then, they will pass the approvedContract() modifier:

 modifier approvedContract() {
        require(contracts[msg.sender], "Contract is not approved to make transfers");
        _;
    }

Which makes all the transfer calls accepted from a new approvedContract:

function transferERC721(address collection, address from, address to, uint256 tokenId)
        approvedContract
        external
    {
        require(revokedApproval[from] == false, "User has revoked approval");
        IERC721(collection).safeTransferFrom(from, to, tokenId);
    }

So we've demonstrated a dead simple path where a malicious / hacked owner can abuse approvals to existing executionDelegate by adding a new evil contract to approved contract list and calling transfer functions on executionDelegate from that contract.

Again there is zero speculation on future code, only a demonstration of how existing code facilitates a rug pull. Speculation would be to state that owner will rug pull the project, we are merely pointing out that owner can rug pull the project.

I think this establishes clearly enough that owner can indeed instantly rug all user funds. Now we can take a look at previous similar findings: https://github.com/code-423n4/2022-06-illuminate-findings/issues/282 - shows admin can approve() malicious tokens and thereby steal user funds. The suggested fix is the same as mine, to put behind a timelock. It's essentially the same impact and similar call to risk 2. https://github.com/code-423n4/2022-10-thegraph-findings/issues/300 - simply stating that Governor has infinite approval on bridge funds is enough for M finding.

Also we can take a look at countless inferior centralization threats marked as M+ : https://gist.github.com/GalloDaSballo/881e7a45ac14481519fb88f34fdb8837 And only recently, a much weaker centralization threat in ArtGobblers was awarded M ( malicious admin can prevent new mints).

Hopefully this makes the case clear enough but would be happy to factually discuss any objection.

Dup of #235

version will be changed when BlurExchange implementation is updated

Possibly, still I wouldn't count on developers bumping the version for each update, more often then not it will just lie around in the codebase. Good observation though!

All upgrades will be made publicly and will give users time to adjust their listings as necessary. Won't be making changes as upgrading the version can still keep listings safe.

Will give a second look, but similarly to _gap like finding, we cannot make conjectures about code that is not in-scope.

I thin that because of the logical similarity to the Gap reports, the finding is valid.

But because we cannot determine a risk based on future code, I will downgraded to QA, Low Severity.

We can do this with recommendation that the sponsor does check the version value on new upgrades, however we cannot comment on the security of code from the future.

L