Fractional v2 contest - 8olidity's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/main/src/Vault.sol#L73

Vulnerability details

Impact

The install function in vault does not detect the value of plugins. As a result, address(0) can be passed in, which conflicts with uninstall

File: src\Vault.sol
  function install(bytes4[] memory _selectors, address[] memory _plugins)
      external
  {
      if (owner != msg.sender) revert NotOwner(owner, msg.sender);
      uint256 length = _selectors.length;
      for (uint256 i = 0; i < length; i++) {
          methods[_selectors[i]] = _plugins[i];
      }
      emit InstallPlugin(_selectors, _plugins);
  }

function uninstall(bytes4[] memory _selectors) external {
    if (owner != msg.sender) revert NotOwner(owner, msg.sender);
    uint256 length = _selectors.length;
    for (uint256 i = 0; i < length; i++) {
        methods[_selectors[i]] = address(0);
    }
    emit UninstallPlugin(_selectors);
}

Proof of Concept

forge test

    function initializeNFTETH()
    public
    view
    returns (bytes4[] memory, address[] memory)
    {
        address[] memory plugins = new address[](3);
        bytes4[] memory selectors = new bytes4[](3);

        (plugins[0], plugins[1], plugins[2]) = (
            address(0),// this is address(0)
            address(0),
            address(0)
        );
        (selectors[0], selectors[1], selectors[2]) = (
            receiver.onERC1155Received.selector,
            receiver.onERC1155BatchReceived.selector,
            receiver.onERC721Received.selector
        );
        return (selectors, plugins);
    }


    function testReceiveERC721() public {
        vaultProxy.init();

        vaultProxy.install(nftReceiverSelectors1, nftReceiverPlugins1);
        MockERC721(erc721).mint(vault, 2);

        assertEq(IERC721(erc721).balanceOf(vault), 1);
    }

In this way, the plugins in _selectors are all 0

Tools Used

Recommended Mitigation Steps

Unused receive() function will lock Ether in contract

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

File: src\Vault.sol:
  31      /// @dev Callback for receiving Ether when the calldata is empty
  32:     receive() external payable {}
  33  

File: src\mocks\MockEthReceiver.sol:
  4  contract MockEthReceiver {
  5:     receive() external payable {}
  6  }

  8  contract MockEthNotReceiver {
  9:     receive() external payable {
  10          revert();

File: src\mocks\MockSender.sol:
  10  
  11:     receive() external payable {}
  12  }

File: src\modules\Buyout.sol:
  52      /// @dev Callback for receiving ether when the calldata is empty
  53:     receive() external payable {}
  54  

File: src\modules\Migration.sol:
  62      /// @dev Callback for receiving ether when the calldata is empty
  63:     receive() external payable {}

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

File: src\constants\Permit.sol:
   4  /// @dev The EIP-712 typehash for the contract's domain
   5: bytes32 constant DOMAIN_TYPEHASH = keccak256(
   6      "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"

   9  /// @dev The EIP-712 typehash for the permit struct used by the contract
  10: bytes32 constant PERMIT_TYPEHASH = keccak256(
  11      "Permit(address owner,address operator,uint256 tokenId,bool approved,uint256 nonce,uint256 deadline)"

  14  /// @dev The EIP-712 typehash for the permit all struct used by the contract
  15: bytes32 constant PERMIT_ALL_TYPEHASH = keccak256(
  16      "PermitAll(address owner,address operator,bool approved,uint256 nonce,uint256 deadline)"

IT COSTS MORE GAS TO INITIALIZE VARIABLES TO ZERO THAN TO LET THE DEFAULT OF ZERO BE APPLIED

File: src\Vault.sol:
   77          uint256 length = _selectors.length;
   78:         for (uint256 i = 0; i < length; i++) {
   79              methods[_selectors[i]] = _plugins[i];

  103          uint256 length = _selectors.length;
  104:         for (uint256 i = 0; i < length; i++) {
  105              methods[_selectors[i]] = address(0);

File: src\modules\protoforms\BaseVault.sol:
   63      ) external {
   64:         for (uint256 i = 0; i < _tokens.length; ) {
   65              IERC20(_tokens[i]).transferFrom(_from, _to, _amounts[i]);

   82      ) external {
   83:         for (uint256 i = 0; i < _tokens.length; ) {
   84              IERC721(_tokens[i]).safeTransferFrom(_from, _to, _ids[i]);

  106          unchecked {
  107:             for (uint256 i = 0; i < _tokens.length; ++i) {
  108                  IERC1155(_tokens[i]).safeTransferFrom(

File: src\utils\MerkleBase.sol:
  50          unchecked {
  51:             for (uint256 i = 0; i < _proof.length; ++i) {
  52                  rollingHash = hashLeafPairs(rollingHash, _proof[i]);

<ARRAY>.LENGTH SHOULD NOT BE LOOKED UP IN EVERY LOOP OF A FOR-LOOP

Even memory arrays incur the overhead of bit tests and bit shifts to calculate the array length. Storage array length checks incur an extra Gwarmaccess (100 gas) PER-LOOP.

File: src\modules\protoforms\BaseVault.sol:
   63      ) external {
   64:         for (uint256 i = 0; i < _tokens.length; ) {
   65              IERC20(_tokens[i]).transferFrom(_from, _to, _amounts[i]);

   82      ) external {
   83:         for (uint256 i = 0; i < _tokens.length; ) {
   84              IERC721(_tokens[i]).safeTransferFrom(_from, _to, _ids[i]);

  106          unchecked {
  107:             for (uint256 i = 0; i < _tokens.length; ++i) {
  108                  IERC1155(_tokens[i]).safeTransferFrom(

  129          unchecked {
  130:             for (uint256 i; i < _modules.length; ++i) {
  131                  bytes32[] memory leaves = IModule(_modules[i]).getLeafNodes();
  132:                 for (uint256 j; j < leaves.length; ++j) {
  133                      hashes[counter++] = leaves[j];

File: src\utils\MerkleBase.sol:
   50          unchecked {
   51:             for (uint256 i = 0; i < _proof.length; ++i) {
   52                  rollingHash = hashLeafPairs(rollingHash, _proof[i]);

  109              uint256 offset;
  110:             for (uint256 i; i < result.length; ++i) {
  111                  if (result[i] != bytes32(0)) {