Blur Exchange contest - minhtrng's results

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/matchingPolicies/StandardPolicyERC1155.sol#L24-L36 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/matchingPolicies/StandardPolicyERC1155.sol#L50-L61

Vulnerability details

Impact

The amount value passed in the order struct is never used. While this is not a problem for ERC721 trades, it can cause a user to attempt to buy multiple tokens with a certain id of an ERC1155 token, but only receiving 1 token in return. A malicious user could also use this to create misleading orders which seem like a bargain, but end up not being such.

Proof of Concept

The order struct defines an amount field which is currently not used anywhere. The amount field is likely meant to be used to specify a token amount for ERC1155 trades, which is also evident by looking at the function signature of ExecutionDelegate.transferERC1155:

function transferERC1155(address collection, address from, address to, uint256 tokenId, uint256 amount)

However, the StandardPolicyERC1155 currently does not check ask and bid orders for a matching amount and returns a fixed value of 1 for the amount:

StandardPolicyERC1155.sol, L. 24-36 (and L. 50-61):

return (
            (makerAsk.side != takerBid.side) &&
            (makerAsk.paymentToken == takerBid.paymentToken) &&
            (makerAsk.collection == takerBid.collection) &&
            (makerAsk.tokenId == takerBid.tokenId) &&
            (makerAsk.matchingPolicy == takerBid.matchingPolicy) &&
            (makerAsk.price == takerBid.price),
            makerAsk.price,
            makerAsk.tokenId,
            1,
            AssetType.ERC1155
        );

This can cause the issue described under Impact.

A slight modification of the "can transfer ERC1155" test case also shows this (all amounts changed from 1 to 2):

it('can transfer ERC1155', async () => {
      await mockERC1155.mint(alice.address, tokenId, 2);
      sell = generateOrder(alice, {
        side: Side.Sell,
        tokenId,
        amount: 2,
        collection: mockERC1155.address,
        matchingPolicy: matchingPolicies.standardPolicyERC1155.address,
      });
      buy = generateOrder(bob, {
        side: Side.Buy,
        tokenId,
        amount: 2,
        collection: mockERC1155.address,
        matchingPolicy: matchingPolicies.standardPolicyERC1155.address,
      });
      sellInput = await sell.pack();
      buyInput = await buy.pack();

      await waitForTx(exchange.execute(sellInput, buyInput));

      expect(await mockERC1155.balanceOf(bob.address, tokenId)).to.be.equal(2); //This assertion fails

Tools Used

Manual review

Recommended Mitigation Steps

In the StandardPolicyERC1155 (for both functions) add an additional check for equality of the amounts and return it instead of the fixed "1":

//...
(makerAsk.price == takerBid.price) &&
(makerAsk.amount == makerAsk.amount),
makerAsk.price,
makerAsk.tokenId,
makerAsk.amount,

This will cause the right amount to be passed in BlurExchange._executeTokenTransfer().

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L36-L39

Vulnerability details

Impact

The ExecutionDelegate has external functions to transfer approved assets from one address to another as long as they are called from an owner approved contract. A potential issue lies in the fact that the owner can approve contracts instantly and without any restrictions, which would allow them to immediately drain all approved assets from users, without them having a chance to react.

Proof of Concept

The transferERCXYZ functions of the ExecutionDelegate are only restricted by the approved modifier and require an address to not have set revokedApproval (which a user would not do if he has active orders and has trust in his assets being safe). The approveContract function is callable by the owner and immediately makes the passed address eligible to call the transferERCXYZ functions, which would allow theft of all assets that are approved to the ExecutionDelegate (e.g. in case of a private key compromise).

Tools Used

Manual review

Recommended Mitigation Steps

Suggestion: Make the approval of contracts in the ExecutionDelegate a 2-step process with the second step being restricted by a timelock of some kind. The emmittance of an event after the first step would notify users about a contract approval being initiated, so they could revoke their asset approvals before the second step can be executed.