Platform: Code4rena
Start Date: 05/10/2022
Pot Size: $50,000 USDC
Total HM: 2
Participants: 80
Period: 5 days
Judge: GalloDaSballo
Id: 168
League: ETH
Rank: 57/80
Findings: 1
Award: $50.48
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: 0x4non
Also found by: 0x1f8b, 0xNazgul, 0xSmartContract, Deivitto, IllIllI, Lambda, RaymondFam, Rolezn, RustyRabbit, Trust, arcoun, bin2chen, brgltd, csanuragjain, d3e4, enckrish, exd0tpy, ladboy233, nicobevi, rbserver, rotcivegaf, simon135, zzykxx
50.4817 USDC - $50.48
ecrcover not checking for 0 valueThe ecrecover function is used in _recover() to recover the address from the signature. The built-in EVM precompile ecrecover is susceptible to signature malleability which could lead to replay attacks (references: https://swcregistry.io/docs/SWC-117, https://swcregistry.io/docs/SWC-121 and https://medium.com/cryptronics/signature-replay-vulnerabilities-in-smart-contracts-3b6f7596df57).
Consider using OpenZeppelin’s ECDSA library (which prevents this malleability) instead of the built-in function.
Fixed gas cost are not good reentrancy mitigations as the cost may change by the time.
Avoid using transfer fixed cost as a reentrancy mitigation as the gas cost may change.
address state or immutable variablesZero address should be checked for state variables, immutable variables. A zero address can lead into problems.
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L113 https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L116
Check zero address before assigning or using it
Risk of using block.timestamp for time should be considered.
block.timestamp is not an ideal proxy for time because of issues with synchronization, miner manipulation and changing block times.
This kind of issue may affect the code allowing or reverting the code before the expected deadline, modifying the normal functioning or reverting sometimes.
SWC ID: 116
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L283 return (listingTime < block.timestamp) && (expirationTime == 0 || block.timestamp < expirationTime);
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L258-L271 ((order.trader != address(0)) && (cancelledOrFilled[orderHash] == false) && _canSettleOrder(order.listingTime,order.expirationTime))
block.timestamp as time proxy and evaluate if block numbers can be used as an approximation for the application logic. Both have risks that need to be factored in.The initialize function that initializes important contract state can be called by anyone.
The attacker can initialize the contract before the legitimate deployer, hoping that the victim continues to use the same contract.
In the best case for the victim, they notice it and have to redeploy their contract costing gas.
Use the constructor to initialize non-proxied contracts.
For initializing proxy contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify the transaction succeeded.
asserts()From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.
With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam.
You have reachable asserts in the following locations (which should be replaced by require / are mistakenly left from development phase):
The Solidity assert() function is meant to assert invariants. Properly functioning code should never reach a failing assert statement. A reachable assertion can mean one of two things:
A bug exists in the contract that allows it to enter an invalid state; The assert statement is used incorrectly, e.g. to validate inputs.
SWC ID: 110
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/lib/ERC1967Proxy.sol#L22 assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1));
Substitute asserts with require/revert.
Return values not being checked may lead into unexpected behaviors with functions. Not events/Error are being emitted if that fails, so functions would be called even of not being working as expect as for [example _addCollateral what would interact with the ether.]
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/PolicyManager.sol#L25-L30 https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/PolicyManager.sol#L36-L41 https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L496-L515
Check values and revert/emit events if needed
Missing Natspec and regular comments affect readability and maintainability of a codebase.
Contracts has partial or full lack of comments
@param and @return valuehttps://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/lib/ReentrancyGuarded.sol https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/lib/ERC1967Proxy.sol https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/PolicyManager.sol https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/matchingPolicies/StandardPolicyERC1155.sol https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/matchingPolicies/StandardPolicyERC721.sol https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/ExecutionDelegate.sol https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/lib/EIP712.sol https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/lib/MerkleVerifier.sol https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/lib/OrderStructs.sol
There are a number of instances where a boolean variable/function is checked.
variable == false to !variable.https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/ExecutionDelegate.sol#L77 require(revokedApproval[from] == false, "User has revoked approval");
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/ExecutionDelegate.sol#L92 require(revokedApproval[from] == false, "User has revoked approval");
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/ExecutionDelegate.sol#L108 require(revokedApproval[from] == false, "User has revoked approval");
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/ExecutionDelegate.sol#L124 require(revokedApproval[from] == false, "User has revoked approval");
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L267 (cancelledOrFilled[orderHash] == false) &&
Simplify boolean comparisons in order to improve readability and save gas
Events without indexed event parameters make it harder and inefficient for off-chain tools to analyze them.
Indexed parameters (“topics”) are searchable event parameters. They are stored separately from unindexed event parameters in an efficient manner to allow for faster access. This is useful for efficient off-chain-analysis, but it is also more costly gas-wise.
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L86 event NonceIncremented(address trader, uint256 newNonce);
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L88 event NewExecutionDelegate(IExecutionDelegate executionDelegate);
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L89 event NewPolicyManager(IPolicyManager policyManager);
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L90 event NewOracle(address oracle);
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L91 event NewBlockRange(uint256 blockRange);
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L41 event Closed();
Consider which event parameters could be particularly useful to off-chain tools and should be indexed.
Constant naming convention is all upper case.
Some constants are not using proper style.
Constant should be in UPPER_CASE_WITH_UNDERSCORES as per Solidity Style Guide.
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L57 string public constant name = "Blur Exchange";
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L58 string public constant version = "1.0";
Rename the constant to uppercase style: CONSTANTS_WITH_UNDERSCORES.
Long lines should be wrapped to conform with Solidity Style guidelines.
Lines that exceed the 79 (or 99) character length suggested by the Solidity Style guidelines. Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
Reduce line length to less than 99 at least to improve maintainability and readability of the code
Require/revert statements should include error messages in order to help at monitoring the system.
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L134 require(sell.order.side == Side.Sell);
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L183 require(msg.sender == order.trader);
https://github.com/code-423n4/2022-10-blur/blob/d1c22a94ed08b08fe3f7d5c96e973d80d3dc0e54/contracts/BlurExchange.sol#L452 require(msg.value == price);
Add error messages
#0 - GalloDaSballo
2022-10-22T22:31:12Z
R
Disagree with the finding, the function is nonReentrant
L
Disagree with blanket statement
L
R
Disagree for instances given
NC
NC
Disagree, some events don't need params, and not all params should be indexed
Disagree with those specific instances
NC
NC
#1 - GalloDaSballo
2022-10-22T22:31:29Z
2L 4NC 1R