search

code-423n4 / 2022-07-fractional-findings

0 stars 0 forks source link

QA Report #631

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Unsafe ERC20 Operation(s)

Information : L001 - Unsafe ERC20 Operation(s)

Instances include :

utils/SafeSend.sol:33:            WETH(WETH_ADDRESS).transfer(_to, _value);
references/TransferReference.sol:22:        IERC20(_token).transfer(_to, _value);
modules/Migration.sol:175:        payable(msg.sender).transfer(ethAmount);
modules/Migration.sol:328:        payable(msg.sender).transfer(userEth);
modules/protoforms/BaseVault.sol:65:            IERC20(_tokens[i]).transferFrom(_from, _to, _amounts[i]);

Recommendation

It is recommended to always use OpenZeppelin's SafeERC20 library, for example :

references/TransferReference.sol:22:        IERC20(_token).safeTransfer(_to, _value);

_safeMint should be used instead of _mint wherever possible

It is encouraged to use _safeMint instead of _mint, as _safeMint ensures that the recipient is an EOA (Externally Owned Account).

Instances include :

FERC1155.sol:85:        _mint(_to, _id, _amount, _data);

Recommendation

It is recommended to use _safeMint instead of _mint wherever possible, for example :

FERC1155.sol:85:        _safeMint(_to, _id, _amount, _data);

Use two-step transfer pattern for access controls

Contracts that implement access control, e.g. owner, should consider implementing a two-step transfer pattern. Otherwise, it is possible that the role mistakenly transfers ownership to the wrong address, resulting in losing the role.

Instances include :

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

Recommendation

It is recommended separate the ownership transfer into 2 functions, first to set a pending owner, and second to approve the pending owner set using the first function and actually implementing the change of the ownership.

Use of block.timestamp

Information : Block timestamp

Instances include :

ERC20.permit(address,address,uint256,uint256,uint8,bytes32,bytes32) (node_modules/@rari-capital/solmate/src/tokens/ERC20.sol#116-160) uses timestamp for comparisons
    Dangerous comparisons:
    - require(bool,string)(deadline >= block.timestamp,PERMIT_DEADLINE_EXPIRED) (node_modules/@rari-capital/solmate/src/tokens/ERC20.sol#125)
FERC1155.permit(address,address,uint256,bool,uint256,uint8,bytes32,bytes32) (src/FERC1155.sol#98-135) uses timestamp for comparisons
    Dangerous comparisons:
    - block.timestamp > _deadline (src/FERC1155.sol#108)
FERC1155.permitAll(address,address,bool,uint256,uint8,bytes32,bytes32) (src/FERC1155.sol#145-180) uses timestamp for comparisons
    Dangerous comparisons:
    - block.timestamp > _deadline (src/FERC1155.sol#154)
Buyout.sellFractions(address,uint256) (src/modules/Buyout.sol#112-144) uses timestamp for comparisons
    Dangerous comparisons:
    - block.timestamp > endTime (src/modules/Buyout.sol#125)
Buyout.buyFractions(address,uint256) (src/modules/Buyout.sol#149-179) uses timestamp for comparisons
    Dangerous comparisons:
    - block.timestamp > endTime (src/modules/Buyout.sol#162)
Buyout.end(address,bytes32[]) (src/modules/Buyout.sol#184-239) uses timestamp for comparisons
    Dangerous comparisons:
    - block.timestamp <= endTime (src/modules/Buyout.sol#203)
Migration.commit(address,uint256) (src/modules/Migration.sol#182-217) uses timestamp for comparisons
    Dangerous comparisons:
    - block.timestamp > proposal.startTime + PROPOSAL_PERIOD (src/modules/Migration.sol#197)
    - currentPrice > proposal.targetPrice (src/modules/Migration.sol#209)
Migration.withdrawContribution(address,uint256) (src/modules/Migration.sol#295-329) uses timestamp for comparisons
    Dangerous comparisons:
    - current != State.INACTIVE || migrationInfo[_vault][_proposalId].newVault != address(0) (src/modules/Migration.sol#306-307)

Recommendation

Avoid relying on block.timestamp.

Calls inside a loop

Information : Calls inside a loop

Instances include :

Multicall.multicall(bytes[]) (src/utils/Multicall.sol#11-35) has external calls inside a loop: (success,result) = address(this).delegatecall(_data[i]) (src/utils/Multicall.sol#21)
Migration.generateMerkleTree(address[]) (src/modules/Migration.sol#490-515) has external calls inside a loop: treeLength += IModule(_modules[i]).getLeafNodes().length (src/modules/Migration.sol#500)
Migration.generateMerkleTree(address[]) (src/modules/Migration.sol#490-515) has external calls inside a loop: leaves = IModule(_modules[i_scope_0]).getLeafNodes() (src/modules/Migration.sol#508)
BaseVault.batchDepositERC20(address,address,address[],uint256[]) (src/modules/protoforms/BaseVault.sol#58-70) has external calls inside a loop: IERC20(_tokens[i]).transferFrom(_from,_to,_amounts[i]) (src/modules/protoforms/BaseVault.sol#65)
BaseVault.batchDepositERC721(address,address,address[],uint256[]) (src/modules/protoforms/BaseVault.sol#77-89) has external calls inside a loop: IERC721(_tokens[i]).safeTransferFrom(_from,_to,_ids[i]) (src/modules/protoforms/BaseVault.sol#84)
BaseVault.batchDepositERC1155(address,address,address[],uint256[],uint256[],bytes[]) (src/modules/protoforms/BaseVault.sol#98-117) has external calls inside a loop: IERC1155(_tokens[i]).safeTransferFrom(_from,_to,_ids[i],_amounts[i],_datas[i]) (src/modules/protoforms/BaseVault.sol#108-114)
BaseVault.generateMerkleTree(address[]) (src/modules/protoforms/BaseVault.sol#122-137) has external calls inside a loop: leaves = IModule(_modules[i]).getLeafNodes() (src/modules/protoforms/BaseVault.sol#131)

Recommendation

Favor pull over push strategy for external calls.

Unused return

Information : Unused return

Instances include :

Buyout.end(address,bytes32[]) (src/modules/Buyout.sol#184-239) ignores return value by IVault(address(_vault)).execute(supply,data,_burnProof) (src/modules/Buyout.sol#219)
Buyout.cash(address,bytes32[]) (src/modules/Buyout.sol#244-273) ignores return value by IVault(address(_vault)).execute(supply,data,_burnProof) (src/modules/Buyout.sol#264)
Buyout.redeem(address,bytes32[]) (src/modules/Buyout.sol#278-303) ignores return value by IVault(address(_vault)).execute(supply,data,_burnProof) (src/modules/Buyout.sol#294)
Buyout.withdrawERC20(address,address,address,uint256,bytes32[]) (src/modules/Buyout.sol#311-335) ignores return value by IVault(address(_vault)).execute(transfer,data,_erc20TransferProof) (src/modules/Buyout.sol#334)
Buyout.withdrawERC721(address,address,address,uint256,bytes32[]) (src/modules/Buyout.sol#343-367) ignores return value by IVault(address(_vault)).execute(transfer,data,_erc721TransferProof) (src/modules/Buyout.sol#366)
Buyout.withdrawERC1155(address,address,address,uint256,uint256,bytes32[]) (src/modules/Buyout.sol#376-404) ignores return value by IVault(address(_vault)).execute(transfer,data,_erc1155TransferProof) (src/modules/Buyout.sol#403)
Buyout.batchWithdrawERC1155(address,address,address,uint256[],uint256[],bytes32[]) (src/modules/Buyout.sol#413-445) ignores return value by IVault(address(_vault)).execute(transfer,data,_erc1155BatchTransferProof) (src/modules/Buyout.sol#440-444)
Minter._mintFractions(address,address,uint256,bytes32[]) (src/modules/Minter.sol#50-61) ignores return value by IVault(address(_vault)).execute(supply,data,_mintProof) (src/modules/Minter.sol#60)

Recommendation

Ensure that all the return values of the function calls are used.

Divide before multiply

Information : Divide before multiply

Instances include :

Supply.mint(address,uint256) (src/targets/Supply.sol#23-103) performs a multiplication on the result of a division:
    -returnDataWords_mint_asm_0 = returndatasize()() / 0x20 (src/targets/Supply.sol#49)
    -cost_mint_asm_0 = 3 * returnDataWords_mint_asm_0 (src/targets/Supply.sol#57)
Supply.mint(address,uint256) (src/targets/Supply.sol#23-103) performs a multiplication on the result of a division:
    -msizeWords_mint_asm_0 = memPointer_mint_asm_0 / 0x20 (src/targets/Supply.sol#54)
    -cost_mint_asm_0 = cost_mint_asm_0 + returnDataWords_mint_asm_0 - msizeWords_mint_asm_0 * 3 + returnDataWords_mint_asm_0 * returnDataWords_mint_asm_0 - msizeWords_mint_asm_0 * msizeWords_mint_asm_0 / 0x200 (src/targets/Supply.sol#61-73)
Supply.burn(address,uint256) (src/targets/Supply.sol#108-188) performs a multiplication on the result of a division:
    -returnDataWords_burn_asm_0 = returndatasize()() / 0x20 (src/targets/Supply.sol#134)
    -cost_burn_asm_0 = 3 * returnDataWords_burn_asm_0 (src/targets/Supply.sol#142)
Supply.burn(address,uint256) (src/targets/Supply.sol#108-188) performs a multiplication on the result of a division:
    -msizeWords_burn_asm_0 = memPointer_burn_asm_0 / 0x20 (src/targets/Supply.sol#139)
    -cost_burn_asm_0 = cost_burn_asm_0 + returnDataWords_burn_asm_0 - msizeWords_burn_asm_0 * 3 + returnDataWords_burn_asm_0 * returnDataWords_burn_asm_0 - msizeWords_burn_asm_0 * msizeWords_burn_asm_0 / 0x200 (src/targets/Supply.sol#146-158)
Transfer.ERC20Transfer(address,address,uint256) (src/targets/Transfer.sol#18-177) performs a multiplication on the result of a division:
    -returnDataWords_ERC20Transfer_asm_0 = returndatasize()() + 0x1f / 0x20 (src/targets/Transfer.sol#75-78)
    -cost_ERC20Transfer_asm_0 = 3 * returnDataWords_ERC20Transfer_asm_0 (src/targets/Transfer.sol#87)
Transfer.ERC20Transfer(address,address,uint256) (src/targets/Transfer.sol#18-177) performs a multiplication on the result of a division:
    -msizeWords_ERC20Transfer_asm_0 = memPointer_ERC20Transfer_asm_0 / 0x20 (src/targets/Transfer.sol#84)
    -cost_ERC20Transfer_asm_0 = cost_ERC20Transfer_asm_0 + returnDataWords_ERC20Transfer_asm_0 - msizeWords_ERC20Transfer_asm_0 * 3 + returnDataWords_ERC20Transfer_asm_0 * returnDataWords_ERC20Transfer_asm_0 - msizeWords_ERC20Transfer_asm_0 * msizeWords_ERC20Transfer_asm_0 / 0x200 (src/targets/Transfer.sol#91-112)
Transfer.ERC721TransferFrom(address,address,address,uint256) (src/targets/Transfer.sol#184-287) performs a multiplication on the result of a division:
    -returnDataWords_ERC721TransferFrom_asm_0 = returndatasize()() + 0x20 / 0x20 (src/targets/Transfer.sol#226-229)
    -cost_ERC721TransferFrom_asm_0 = 3 * returnDataWords_ERC721TransferFrom_asm_0 (src/targets/Transfer.sol#237)
Transfer.ERC721TransferFrom(address,address,address,uint256) (src/targets/Transfer.sol#184-287) performs a multiplication on the result of a division:
    -msizeWords_ERC721TransferFrom_asm_0 = memPointer_ERC721TransferFrom_asm_0 / 0x20 (src/targets/Transfer.sol#234)
    -cost_ERC721TransferFrom_asm_0 = cost_ERC721TransferFrom_asm_0 + returnDataWords_ERC721TransferFrom_asm_0 - msizeWords_ERC721TransferFrom_asm_0 * 3 + returnDataWords_ERC721TransferFrom_asm_0 * returnDataWords_ERC721TransferFrom_asm_0 - msizeWords_ERC721TransferFrom_asm_0 * msizeWords_ERC721TransferFrom_asm_0 / 0x200 (src/targets/Transfer.sol#241-253)
Transfer.ERC1155TransferFrom(address,address,address,uint256,uint256) (src/targets/Transfer.sol#295-408) performs a multiplication on the result of a division:
    -returnDataWords_ERC1155TransferFrom_asm_0 = returndatasize()() + 0x20 / 0x20 (src/targets/Transfer.sol#343-346)
    -cost_ERC1155TransferFrom_asm_0 = 3 * returnDataWords_ERC1155TransferFrom_asm_0 (src/targets/Transfer.sol#354)
Transfer.ERC1155TransferFrom(address,address,address,uint256,uint256) (src/targets/Transfer.sol#295-408) performs a multiplication on the result of a division:
    -msizeWords_ERC1155TransferFrom_asm_0 = memPointer_ERC1155TransferFrom_asm_0 / 0x20 (src/targets/Transfer.sol#351)
    -cost_ERC1155TransferFrom_asm_0 = cost_ERC1155TransferFrom_asm_0 + returnDataWords_ERC1155TransferFrom_asm_0 - msizeWords_ERC1155TransferFrom_asm_0 * 3 + returnDataWords_ERC1155TransferFrom_asm_0 * returnDataWords_ERC1155TransferFrom_asm_0 - msizeWords_ERC1155TransferFrom_asm_0 * msizeWords_ERC1155TransferFrom_asm_0 / 0x200 (src/targets/Transfer.sol#358-370)
Transfer.ERC1155BatchTransferFrom(address,address,address,uint256[],uint256[]) (src/targets/Transfer.sol#411-580) performs a multiplication on the result of a division:
    -returnDataWords_ERC1155BatchTransferFrom_asm_0 = returndatasize()() + 0x20 / 0x20 (src/targets/Transfer.sol#512-515)
    -cost_ERC1155BatchTransferFrom_asm_0 = 3 * returnDataWords_ERC1155BatchTransferFrom_asm_0 (src/targets/Transfer.sol#528)
Transfer.ERC1155BatchTransferFrom(address,address,address,uint256[],uint256[]) (src/targets/Transfer.sol#411-580) performs a multiplication on the result of a division:
    -msizeWords_ERC1155BatchTransferFrom_asm_0 = transferDataSize_ERC1155BatchTransferFrom_asm_0 / 0x20 (src/targets/Transfer.sol#525)
    -cost_ERC1155BatchTransferFrom_asm_0 = cost_ERC1155BatchTransferFrom_asm_0 + returnDataWords_ERC1155BatchTransferFrom_asm_0 - msizeWords_ERC1155BatchTransferFrom_asm_0 * 3 + returnDataWords_ERC1155BatchTransferFrom_asm_0 * returnDataWords_ERC1155BatchTransferFrom_asm_0 - msizeWords_ERC1155BatchTransferFrom_asm_0 * msizeWords_ERC1155BatchTransferFrom_asm_0 / 0x200 (src/targets/Transfer.sol#532-544)

Recommendation

Consider ordering multiplication before division.

Reentrancy vulnerabilities

Information : reentrancy-events reentrancy-benign reentrancy-no-eth reentrancy-eth check-effects-interactions pattern

Instances include :

Reentrancy in Migration.commit(address,uint256) (src/modules/Migration.sol#182-217):
    External calls:
    - IFERC1155(token).setApprovalFor(address(buyout),id,true) (src/modules/Migration.sol#211)
    - IBuyout(buyout).start{value: proposal.totalEth}(_vault) (src/modules/Migration.sol#213)
    External calls sending eth:
    - IBuyout(buyout).start{value: proposal.totalEth}(_vault) (src/modules/Migration.sol#213)
    State variables written after the call(s):
    - proposal.isCommited = true (src/modules/Migration.sol#214)
Reentrancy in VaultFactory.deployFor(address) (src/VaultFactory.sol#62-89):
    External calls:
    - Vault(vault).init() (src/VaultFactory.sol#70)
    - Vault(vault).transferOwnership(_owner) (src/VaultFactory.sol#73)
    State variables written after the call(s):
    - nextSeeds[tx.origin] = bytes32(uint256(seed) + 1) (src/VaultFactory.sol#77)
Reentrancy in Migration.join(address,uint256,uint256) (src/modules/Migration.sol#108-139):
    External calls:
    - IFERC1155(token).safeTransferFrom(msg.sender,address(this),id,_amount,) (src/modules/Migration.sol#129-135)
    State variables written after the call(s):
    - proposal.totalFractions += _amount (src/modules/Migration.sol#137)
Reentrancy in Migration.settleFractions(address,uint256,bytes32[]) (src/modules/Migration.sol#260-290):
    External calls:
    - _mintFractions(proposal.newVault,address(this),proposal.newFractionSupply,_mintProof) (src/modules/Migration.sol#275-280)
        - data = abi.encodeCall(ISupply.mint,(_to,_fractionSupply)) (src/modules/Minter.sol#56-59)
        - IVault(address(_vault)).execute(supply,data,_mintProof) (src/modules/Minter.sol#60)
    State variables written after the call(s):
    - migrationInfo[_vault][_proposalId].fractionsMigrated = true (src/modules/Migration.sol#282)
Reentrancy in Migration.settleVault(address,uint256) (src/modules/Migration.sol#223-254):
    External calls:
    - newVault = IVaultRegistry(registry).create(merkleRoot,proposal.plugins,proposal.selectors) (src/modules/Migration.sol#238-242)
    State variables written after the call(s):
    - proposal.newVault = newVault (src/modules/Migration.sol#244)

Recommendation

Apply the check-effects-interactions pattern.

HardlyDifficult commented 2 years ago

Merging with #643