search

code-423n4 / 2021-10-ambire-findings

0 stars 0 forks source link

Hex selector #66

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

pauliax

Vulnerability details

Impact

You can use .selector instead of a hex number, e.g.: // bytes4(keccak256("isValidSignature(bytes32,bytes)")) bytes4 constant internal ERC1271_MAGICVALUE_BYTES32 = 0x1626ba7e;

is equivavelent to: IERC1271Wallet.isValidSignature.selector

Same can be applied here: method == 0x150b7a02 // bytes4(keccak256("onERC721Received(address,address,uint256,bytes)")) || method == 0xf23a6e61 // bytes4(keccak256("onERC1155Received(address,address,uint256,uint256,bytes)")) || method == 0xbc197c81 // bytes4(keccak256("onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)"))

Recommended Mitigation Steps

Consider using selectors to improve the readability and maintanability.

Ivshti commented 2 years ago

While this is a very reasonable suggestion, we don't want to add all of the additional interfaces at this point. We may reconsider though - thanks for the suggestion!

GalloDaSballo commented 2 years ago

Agree with the finding and like the idea of using .selector it's a non-critical change so no issue if not implemented