QA Report

[code423n4]

QA report

Non-critical

[N-01] Unused imports

Remove this imports:

• IBaseVault.sol:L04 import {IModule} from "./IModule.sol";
• IMigration.sol:L04 import {Permission} from "./IVaultRegistry.sol";
• IMigration.sol:L05 import {State} from "./IBuyout.sol";
• Migration.sol:L04 remove Auction
• Migration.sol:L05 import {IERC20} from "../interfaces/IERC20.sol";
• Migration.sol:L06 import {IERC721} from "../interfaces/IERC721.sol";
• Migration.sol:L07 import {IERC1155} from "../interfaces/IERC1155.sol";
• Migration.sol:L11 import {IVault} from "../interfaces/IVault.sol";
• Supply.sol:L05 import {IVaultRegistry} from "../interfaces/IVaultRegistry.sol";
• Transfer.sol:L04 import {IERC20} from "../interfaces/IERC20.sol";
• Transfer.sol:L05 import {IERC721} from "../interfaces/IERC721.sol";
• Transfer.sol:L06 import {IERC1155} from "../interfaces/IERC1155.sol";

Remove IProtoform.sol:L05 import {Permission} from "./IVaultRegistry.sol"; and in BaseVault.sol:L8 import the IModule from IModule.sol, import {IModule} from "../../interfaces/IModule.sol";

[N-02] Remove payable cast

Remove unnecessary cast from address to payable address:

• Buyout.sol:

L219: IVault(payable(_vault)).execute(supply, data, _burnProof);
L264: IVault(payable(_vault)).execute(supply, data, _burnProof);
L294: IVault(payable(_vault)).execute(supply, data, _burnProof);
L334: IVault(payable(_vault)).execute(transfer, data, _erc20TransferProof);
L366: IVault(payable(_vault)).execute(transfer, data, _erc721TransferProof);
L403: IVault(payable(_vault)).execute(transfer, data, _erc1155TransferProof);
L440: IVault(payable(_vault)).execute(

• Minter.sol, L60: IVault(payable(_vault)).execute(supply, data, _mintProof);

IVault(payable(_vault)) to IVault(_vault)

• In Migration.sol:
  L37 address payable public buyout; to address public buyout;
  L58 buyout = payable(_buyout); to buyout = _buyout;
  and IMigration.sol L94 function buyout() external view returns (address payable); to function buyout() external view returns (address);

• In VaultFactory.sol, L87: address(vault) to vault

[N-03] Use "" instead of abi.encodePacked()

In VaultFactory.sol:
L40: .cloneCreationCode(abi.encodePacked()); to .cloneCreationCode("");
L68-L69:

bytes memory data = abi.encodePacked();
vault = implementation.clone(salt, data);

To

vault = implementation.clone(salt, "");

[N-04] Low accuracy when calculates royaltyAmount

In the FERC1155 contract the royaltyInfo equation have a low accuracy, only 100, add more digits in the BASE and avoid magic numbers

[N-04] The leafNodes are limited by 6

In BaseVault.sol the _modules are limited to total 6 leafNodes: hashes = new bytes32[](6);

[N-05] The multicall function of Multicall.sol its not payable as Uniswap Multicall

Take in mind that since multicall it's top payable you couldn't do a multicall of payable functions.

Multicall.sol

• [N] L11 tendria que ser payable?? la de Uniswap lo es, el de makerdao no lo es pero usa .call en vez de .delegatecall

Low Risk

[L-01] Critical function setContractURI should emit event

L198 setContractURI

[L-02] Missing input validation on array lengths

On calling with arrays of different lengths various malfunctions are possible as the arrays are used as given. Recommendation is to require to all array match in length

BaseVault.sol#L61-L62
BaseVault.sol#L80-L81
BaseVault.sol#L101-L104

[L-03] WETH transfer return values not checked

On SafeSend.sol#L33 the WETH.transfer() and WETH.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success.

We recommend checking the success boolean of all .transfer and .transferFrom calls;

diff --git a/src/utils/SafeSend.sol b/src/utils/SafeSend.sol
index 60d333a..057eade 100644
--- a/src/utils/SafeSend.sol
+++ b/src/utils/SafeSend.sol
@@ -30,7 +30,8 @@ abstract contract SafeSend {
     function _sendEthOrWeth(address _to, uint256 _value) internal {
         if (!_attemptETHTransfer(_to, _value)) {
             WETH(WETH_ADDRESS).deposit{value: _value}();
-            WETH(WETH_ADDRESS).transfer(_to, _value);
+            require(WETH(WETH_ADDRESS).transfer(_to, _value), "Failed to send WETH");
         }
     }
 }

[L-04] Critical function setMerkleRoot on Vault.sol doesnt emit event

Function setMerkleRoot on Vault.sol#L86-L89 should emit event

[L-05] Wrong comment more than 51%

In the Buyout.sol#L21 contract the documentation says: /// - If a pool has more than 51% of the total supply after 4 days, the buyout is successful and the proposer but the L211 have 500 this represent more than 50%
Use 510instead of 500 or change the documentation to more than 50%

[L-06] Don't return success in execute function

In Vault contract, execute function: The success returned from _execute function always will be true in other case the _execute function reverts, even remove the return success parameter of _execute function

[L-07] Assign the WETH in the constructor

The WETH may change between the chains, send the WETH address as a parameter of the constructor and assign it there

[L-08] A malicius plugin on Vault could mess up with state

The _execute function on Vault.sol#L115 runs a delegatecall, after the delegatecall it checks if module has change the owner on Vault.sol#L132, but a malicius plugin could also change the state variable merkleRoot, nonce or add a plugin adding var to the mapping methods and more

[L-09] Royalties with high percentage

In the FERC1155 contract the a controller can set a high royalty percentage with the setRoyalties function
Add a require to check the max royalty percentage

[L-10] Owner has full control and can rug

By definition the owner of the contract has total control of assets and can hard rug all users, consider limit owner priviliges and or a timelock mechanism on critical assets management functions

[HardlyDifficult]

Merging with #426, #546