Blur Exchange contest - RustyRabbit's results

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/matchingPolicies/StandardPolicyERC1155.sol#L59 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L145 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L159

Vulnerability details

Impact

When a user signs an order to buy multiple ERC1155 tokens the amount of tokens transferred is hardcoded to be 1. This means although they expected to get n number of tokens for the total price of X they only get 1 and pay the total amount of X ETH.

Proof of Concept

The canMatchMakerBid() in StandardPolicyERC1155.sol always returns 1 for the amount of tokens matched. This amount is then interpreted to be the amount of tokens to transfer to the buyer, regardless of the amount signed in the order (by the buyer and/or the seller). The price specified in the signed order refers to the total price of the total amount of tokens confirmed by the check that the ETH should be equal to the price in _executeFundsTransfer()

Recommended Mitigation Steps

In the functions canMatchMakerBid() and canMatchMakerAsk() of StandardPolicyERC1155.sol return the correct amount specified in the signed orders and add a check to verify they are equal in both orders.

Lines of code

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

Vulnerability details

Impact

The owner of ExecutionDelegate.solcan specify any EOA to be allowed to transfer user's approved NFTs and WETH.

Proof of Concept

The approveContract() function has an onlyOwner modifier but no delay as to when the new approved address comes into effect. This allows the owner to set an allowed address (can even be a EOA) and from this address transfer all the user's NFTs and WETH they have approved for the contract. This can simply be done by calling the transferERC721(), transferERC1155() and transferERC20() functions as they are guarded by the approvedContract modifier allowing the just added address to call these functions.

Recommended Mitigation Steps

Add a delay before the new address comes into effect so users at least have the opportunity to remove the approval when they realize an inappropriate address is allowed. Alternatively extend the behavior of the revokedApproval mechanism to also specify which external contract can transfer on their behalf and defaulting to true for any contract added after the intialization. (ie; any new contract would have to be allowed specifically by each user).

Low severity

State variable set in constructor of implementation contract rather than initializer.

The initial value of the reentrancyLock state variable used for the reentrancy guard is set during construction of the implementation contract ReentrancyGuarded.sol#L10 This means the default value is not set for the proxy contract as the constructor of the implementation contract is never executed in the proxy contract (including setting default state variables outside the constructor).

Luckily the default value for a boolean is false which mitigates this issue. (and makes this a Low finding rather than Med/High)

It is however recommended to correct this by using an initialize() function in the implementation contract and call it during the initialization of the proxy contract. This limit the chance that if the implementation is altered in the future with other state variables the same mistake is made with possibly far greater impact.

Non-critical

transferERC721Unsafe() is never used

The ``transferERC721Unsafe()` function in [ExecutionDelegate.sol] (https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L73) is never used. Consider removing it.