Fractional v2 contest - Kaiziron's results

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.

---

Don't explicitly initialize variables with the default value

Uninitialized variables are assigned with the default value of their type, initializing a variable with its default value costs unnecessary gas.

Instances include :

utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i) {
modules/protoforms/BaseVault.sol:64:        for (uint256 i = 0; i < _tokens.length; ) {
modules/protoforms/BaseVault.sol:83:        for (uint256 i = 0; i < _tokens.length; ) {
modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) {
Vault.sol:78:        for (uint256 i = 0; i < length; i++) {
Vault.sol:104:        for (uint256 i = 0; i < length; i++) {

Recommendation

It is recommended to initialize variables without assigning them the default value, for example :

utils/MerkleBase.sol:51:            for (uint256 i; i < _proof.length; ++i) {

---

Cache array length outside of for loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Instances include :

utils/MerkleBase.sol:51:            for (uint256 i = 0; i < _proof.length; ++i) {
utils/MerkleBase.sol:62:        require(_data.length > 1, "wont generate root for single leaf");
utils/MerkleBase.sol:78:        require(_data.length > 1, "wont generate proof for single leaf");
utils/MerkleBase.sol:110:            for (uint256 i; i < result.length; ++i) {
modules/protoforms/BaseVault.sol:64:        for (uint256 i = 0; i < _tokens.length; ) {
modules/protoforms/BaseVault.sol:83:        for (uint256 i = 0; i < _tokens.length; ) {
modules/protoforms/BaseVault.sol:107:            for (uint256 i = 0; i < _tokens.length; ++i) {
modules/protoforms/BaseVault.sol:130:            for (uint256 i; i < _modules.length; ++i) {
modules/protoforms/BaseVault.sol:132:                for (uint256 j; j < leaves.length; ++j) {
modules/Buyout.sol:454:        for (uint256 i; i < permissions.length; ) {

Recommendation

It is recommended to cache the array length on a variable before running the loop, then it doesn't need to read the length on every iteration, which cost gas, for example :

uint256 len =  _proof.length;
utils/MerkleBase.sol:51:            for (uint256 i = 0; i < len; ++i) {

---

If possible, use bitwise shift instead of division/multiplication

If the divisor/multiplier x is a power of 2, it can be calculated by shifting log2(x) to the right/left. Division with / cost more gas compared to bitwise shifting.

Instances include :

utils/MerkleBase.sol:100:                _node = _node / 2;
utils/MerkleBase.sol:136:                result = new bytes32[](length / 2 + 1);
utils/MerkleBase.sol:142:                result = new bytes32[](length / 2);

Recommendation

It is recommended to replace / and * with >> and << respectively and divisor/multiplier x to log2(x), for division/multiplication where the divisor/multiplier is a power of 2, for example :

utils/MerkleBase.sol:100:                _node = _node >> 1;

---

If possible, use prefix increment instead of postfix increment

Prefix increment ++i returns the updated value after it's incremented and postfix increment i++ returns the original value then increments it. Prefix increment costs less gas compared to postfix increment.

Instances includes :

utils/MerkleBase.sol:188:                ceil++;
FERC1155.sol:339:                    nonces[_owner]++,
FERC1155.sol:363:                    nonces[_owner]++,
Vault.sol:78:        for (uint256 i = 0; i < length; i++) {
Vault.sol:104:        for (uint256 i = 0; i < length; i++) {

Recommendation

It is recommended to use prefix increment instead of postfix one when the return value is not needed, as both of them will give the same result and prefix increment costs less gas.

For example :

utils/MerkleBase.sol:188:                ++ceil;

---

Don't use long revert strings

Shortening revert strings to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

Instances includes :

utils/MerkleBase.sol:62:        require(_data.length > 1, "wont generate root for single leaf");
utils/MerkleBase.sol:78:        require(_data.length > 1, "wont generate proof for single leaf");

Recommendation

It is recommended to use error code and providing a reference to the error code instead of a long revert string., for example :

// Link to the reference of error codes
utils/MerkleBase.sol:78:        require(_data.length > 1, "ERR");

---

Public function could be declared external

Public functions that are never called by the contract should be declared external to save gas.

Instances includes :

verifyProof(bytes32,bytes32[],bytes32) should be declared external:
	- MerkleBase.verifyProof(bytes32,bytes32[],bytes32) (src/utils/MerkleBase.sol#43-56)
uri(uint256) should be declared external:
	- Metadata.uri(uint256) (src/utils/Metadata.sol#39-41)
selfPermit(address,uint256,bool,uint256,uint8,bytes32,bytes32) should be declared external:
	- SelfPermit.selfPermit(address,uint256,bool,uint256,uint8,bytes32,bytes32) (src/utils/SelfPermit.sol#18-37)
selfPermitAll(address,bool,uint256,uint8,bytes32,bytes32) should be declared external:
	- SelfPermit.selfPermitAll(address,bool,uint256,uint8,bytes32,bytes32) (src/utils/SelfPermit.sol#46-63)

Recommendation

Use the external attribute for functions never called from the contract.

---