Phenix MultiSig

Smart Contract Audit Report

Audit Summary

Phenix MultiSig Audit Report Phenix Finance is creating a new platform in which users can easily create and deploy their own MultiSig contracts.

For this audit, we reviewed Phenix Finance's PhenixMultiSigFactory and PhenixMultiSig contracts at commit b61ee9e7cc243860894a22ac64e5e5251e3b2e0a on the team's GitHub repository.

We previously reviewed the project team's token and vesting contracts here.

Audit Findings

A Medium finding remains present and has been acknowledged by the project team. In addition, centralized aspects are present.
Date: August 18th, 2022.
Updated: September 4th, 2022 to reflect changes from commit d1edd3f2bb6ece5cf24e5f8353e03917bafa6f07 to commit b61ee9e7cc243860894a22ac64e5e5251e3b2e0a.

Finding #1 - PhenixMultiSig - High (Resolved)

Description: Signers of a passed signature list are not checked to ensure that they have not already confirmed a given transaction.
Risk/Impact: A signature list containing duplicate signatures can be passed to bypass the contract's minimum signature requirements. An owner's confirmation can also be counted twice if they confirm using the confirmTransaction() function and subsequently pass their signature to the confirmAndExecuteTransaction() function.
Recommendation: The signer of each signature should be checked to ensure that they have not already granted a confirmation to the transaction.
Resolution: A signature can no longer be used multiple times. In addition, the confirmTransaction function has been removed.

Finding #2 - PhenixMultiSigFactory - High (Resolved)

Description: The canPayTokenFee() function does not take into account if the specified Type is in USDC mode, which would affect the required payment token balance and allowance.
Risk/Impact: Users may be unable to create a PhenixMultiSigWallet despite granting the correct allowance and having a sufficient token balance.
Recommendation: The two userCost() calls in the canPayTokenFee() function should each be updated to the following: 
userCost(getFeeTokenAmount(_type), msg.sender)
Resolution: The project team has implemented the above recommendation.

Finding #3 - PhenixMultiSigFactory - Medium

Description: The getFeeTokenAmount() and getFeeETHAmount() functions both query liquidity pool reserve amounts in order to calculate prices, which can lead to price manipulation.
Risk/Impact: A flash loan could be executed in order to manipulate liquidity pool reserves before creating a PhenixMultiSig contract through the PhenixMultiSigFactory. This can result in the calculated price to be lower than intended.
Recommendation: The team should use a TWAP or Chainlink in order to fetch more reliable prices.
Update:The team has acknowledged and elected not to address this finding.

Finding #4 - PhenixMultiSigFactory - Medium (Resolved)

Description: When paying to generate a new PhenixMultiSig contract using ETH, any excess ETH sent is not returned to the user.
Risk/Impact: Users will lose any excess payment sent to generate a new PhenixMultiSig contract. Users may not know their payment amount as ETH fees are calculated at the time of generation, so losses will likely occur frequently.
Recommendation: Any excess ETH provided by users should be returned to them.
Resolution: The project team has implemented the above recommendation.

Finding #5 - PhenixMultiSig - Low (Resolved)

Description: The following functions contain unbounded loops which iterate through the contract's entire transaction list:
numberOfConfirmedTransactions(), getConfirmedTransactions(), getPendingTransaction(), numberOfPendingTransactions(), getTransactionsInfo(), getExecutedTransactions(), numberOfExecutedTransactions()
Risk/Impact: If the list of total transactions becomes too large, these functions will fail when called due to gas limitations if called during a state-changing transaction.
Recommendation: The data fetched by these functions should be stored in a way that does not require the full transaction list to be iterated through.
Resolution: These functions have been removed. Users can now retrieve a specified number of the most recent transactions.

Finding #6 - PhenixMultiSigFactory - Informational (Resolved)

Description: The _generateMultiSigWallet() function adds newly created PhenixMultiSig addresses to the deployed contracts list twice.
Risk/Impact: The numberOfDeployedContracts() function will return twice the expected value.
Recommendation: The team should only add a newly created address to the deployed contract list once.
Resolution: The project team has implemented the above recommendation.

Contracts Overview

  • As the contracts are implemented with Solidity v0.8.x, they are safe from any possible overflows/underflows.
PhenixMultiSig Contract:
  • This contract is used to allow its owners to propose, approve, and execute transactions.
  • Upon creation, a list of owners and minimum number of confirmations required are passed.
  • Any owner can submit a new transaction at any time.
  • A transaction consists of the address which will be called, an ETH value, the data that will be executed, and information about the transaction.
  • After a transaction is submitted, any owner can execute it by passing in a list of signatures from individual owners to the confirmAndExecuteTransaction() function.
  • Multiple unconfirmed transactions can exist at a time.
  • A transaction must receive the minimum number of required confirmations in order to be executed.
  • If an owner has confirmed a transaction, they are permitted to revoke their confirmation at any time as long as the transaction has not been executed.
  • An owner can reject a transaction at any time, preventing it from being executed regardless of confirmations.
PhenixMultiSigFactory Contract:
  • This contract allows users to create and deploy their own PhenixMultiSig contracts with a specified name, list of owners, and minimum number of required confirmations.
  • When creating a new MultiSig, the user will specify the desired "Type".
  • Each MultiSig Type has its own creation fee. Payments can be supported in either ETH or in the contract's specified payment token.
  • If a MultiSig Type has USDC mode enabled, the token or ETH fee amount will be treated as its USDC price. This price will then be converted to the equivalent payment token or ETH price depending on the user's chosen payment.
  • If a user holds a balance of the contract's specified NFT, their price will be reduced by the contract's "ERC721 discount percentage".
  • If the user is a Factory Admin, they are not subject to fees.
  • The owner can toggle the ability for users to create MultiSig contracts at any time.
  • The owner can update the USDC Pair and Router addresses used for price fetching at any time.
  • The owner can add or remove a Factory Admin at any time.
  • The owner can update the ERC721 discount percentage to any amount at any time.
  • The owner can update the payment token address at any time.
  • The owner can update the Type of any created MultiSigWallet at any time.
  • The owner can update fees for any MultiSig Type, add or remove an accepted payment method, and toggle USDC mode at any time.
  • The owner can withdraw all collected ETH and payment token fees from the contract at any time.

Audit Results

Vulnerability Category Notes Result
Arbitrary Jump/Storage Write N/A PASS
Centralization of Control
  • The owner has the permissions described above.
  • The owner can update the Type of any created PhenixMultiSig contract at any time.
  • The owner can update the fees for any PhenixMultiSig Type at any time, and can also add or remove support for either payment method at any time.
  • The owner can update the PhenixMultiSigFactory's payment token address at any time.
  • An owner of a PhenixMultiSig contract can reject a submitted transaction at any time, preventing it from being executed.
    		•  WARNING
    Compiler Issues N/A PASS
    Delegate Call to Untrusted Contract N/A PASS
    Dependence on Predictable Variables N/A PASS
    Ether/Token Theft N/A PASS
    Flash Loans Flash loans can be used to manipulate PhenixMultiSigFactory fee calculations as described in Finding #3. WARNING
    Front Running N/A PASS
    Improper Events N/A PASS
    Improper Authorization Scheme N/A PASS
    Integer Over/Underflow N/A PASS
    Logical Issues N/A PASS
    Oracle Issues N/A PASS
    Outdated Compiler Version N/A PASS
    Race Conditions N/A PASS
    Reentrancy N/A PASS
    Signature Issues N/A PASS
    Unbounded Loops N/A PASS
    Unused Code N/A PASS
    Overall Contract Safety   PASS

    PhenixMultiSig Contract

    Smart Contract Audit - Inheritance

    Smart Contract Audit - Graph

    
 ($) = payable function
 # = non-constant function
 
 Int = Internal
 Ext = External
 Pub = Public
 
 + [Lib] SafeMath 
    - [Int] tryAdd
    - [Int] trySub
    - [Int] tryMul
    - [Int] tryDiv
    - [Int] tryMod
    - [Int] add
    - [Int] sub
    - [Int] mul
    - [Int] div
    - [Int] mod
    - [Int] sub
    - [Int] div
    - [Int] mod

 +  PhenixMultiSig 
    - [Pub]  #
    - [Ext]  ($)
    - [Pub] submitTransaction #
       - modifiers: onlyOwner
    - [Pub] confirmTransaction #
       - modifiers: onlyOwner,txExists,notExecuted,notConfirmed,notRejected
    - [Int] _confirmTransaction #
    - [Ext] confirmAndExecuteTransaction #
       - modifiers: onlyOwner,txExists,notExecuted,notRejected
    - [Pub] executeTransaction #
       - modifiers: onlyOwner,txExists,notExecuted,notRejected
    - [Int] _executeTransaction #
    - [Pub] revokeConfirmation #
       - modifiers: onlyOwner,txExists,notExecuted
    - [Pub] rejectTransaction #
       - modifiers: onlyOwner,txExists,notExecuted
    - [Pub] getOwners
    - [Pub] getTransactionCount
    - [Ext] deposit ($)
    - [Ext] getTransactions
    - [Ext] getBalance
    - [Pub] numberOfPendingTransactions
    - [Ext] getPendingTransaction
    - [Pub] numberOfExecutedTransactions
    - [Ext] getExecutedTransactions
    - [Pub] numberOfConfirmedTransactions
    - [Ext] getConfirmedTransactions
    - [Ext] getTransactionsInfo
    - [Pub] getTransaction
    - [Pub] getMessageHash
    - [Pub] recoverSigner
    - [Pub] verify
    - [Pub] getEthSignedMessageHash
    - [Pub] splitSignature

    PhenixMultiSigFactory Contract

    Smart Contract Audit - Inheritance

    Smart Contract Audit - Graph

    
 ($) = payable function
 # = non-constant function
 
 Int = Internal
 Ext = External
 Pub = Public
 
 + [Lib] SafeMath 
    - [Int] tryAdd
    - [Int] trySub
    - [Int] tryMul
    - [Int] tryDiv
    - [Int] tryMod
    - [Int] add
    - [Int] sub
    - [Int] mul
    - [Int] div
    - [Int] mod
    - [Int] sub
    - [Int] div
    - [Int] mod

 +  PhenixMultiSig 
    - [Pub]  #
    - [Ext]  ($)
    - [Pub] submitTransaction #
       - modifiers: onlyOwner
    - [Pub] confirmTransaction #
       - modifiers: onlyOwner,txExists,notExecuted,notConfirmed,notRejected
    - [Int] _confirmTransaction #
    - [Ext] confirmAndExecuteTransaction #
       - modifiers: onlyOwner,txExists,notExecuted,notRejected
    - [Pub] executeTransaction #
       - modifiers: onlyOwner,txExists,notExecuted,notRejected
    - [Int] _executeTransaction #
    - [Pub] revokeConfirmation #
       - modifiers: onlyOwner,txExists,notExecuted
    - [Pub] rejectTransaction #
       - modifiers: onlyOwner,txExists,notExecuted
    - [Pub] getOwners
    - [Pub] getTransactionCount
    - [Ext] deposit ($)
    - [Ext] getTransactions
    - [Ext] getBalance
    - [Pub] numberOfPendingTransactions
    - [Ext] getPendingTransaction
    - [Pub] numberOfExecutedTransactions
    - [Ext] getExecutedTransactions
    - [Pub] numberOfConfirmedTransactions
    - [Ext] getConfirmedTransactions
    - [Ext] getTransactionsInfo
    - [Pub] getTransaction
    - [Pub] getMessageHash
    - [Pub] recoverSigner
    - [Pub] verify
    - [Pub] getEthSignedMessageHash
    - [Pub] splitSignature

 + [Int] IUniswapV2Router01 
    - [Ext] factory
    - [Ext] WETH
    - [Ext] addLiquidity #
    - [Ext] addLiquidityETH ($)
    - [Ext] removeLiquidity #
    - [Ext] removeLiquidityETH #
    - [Ext] removeLiquidityWithPermit #
    - [Ext] removeLiquidityETHWithPermit #
    - [Ext] swapExactTokensForTokens #
    - [Ext] swapTokensForExactTokens #
    - [Ext] swapExactETHForTokens ($)
    - [Ext] swapTokensForExactETH #
    - [Ext] swapExactTokensForETH #
    - [Ext] swapETHForExactTokens ($)
    - [Ext] quote
    - [Ext] getAmountOut
    - [Ext] getAmountIn
    - [Ext] getAmountsOut
    - [Ext] getAmountsIn

 + [Int] IUniswapV2Router02 (IUniswapV2Router01)
    - [Ext] removeLiquidityETHSupportingFeeOnTransferTokens #
    - [Ext] removeLiquidityETHWithPermitSupportingFeeOnTransferTokens #
    - [Ext] swapExactTokensForTokensSupportingFeeOnTransferTokens #
    - [Ext] swapExactETHForTokensSupportingFeeOnTransferTokens ($)
    - [Ext] swapExactTokensForETHSupportingFeeOnTransferTokens #

 + [Int] IUniswapV2Pair 
    - [Ext] name
    - [Ext] symbol
    - [Ext] decimals
    - [Ext] totalSupply
    - [Ext] balanceOf
    - [Ext] allowance
    - [Ext] approve #
    - [Ext] transfer #
    - [Ext] transferFrom #
    - [Ext] DOMAIN_SEPARATOR
    - [Ext] PERMIT_TYPEHASH
    - [Ext] nonces
    - [Ext] permit #
    - [Ext] MINIMUM_LIQUIDITY
    - [Ext] factory
    - [Ext] token0
    - [Ext] token1
    - [Ext] getReserves
    - [Ext] price0CumulativeLast
    - [Ext] price1CumulativeLast
    - [Ext] kLast
    - [Ext] mint #
    - [Ext] burn #
    - [Ext] swap #
    - [Ext] skim #
    - [Ext] sync #
    - [Ext] initialize #

 +  Context 
    - [Int] _msgSender
    - [Int] _msgData

 +  Ownable (Context)
    - [Pub]  #
    - [Pub] owner
    - [Int] _checkOwner
    - [Pub] renounceOwnership #
       - modifiers: onlyOwner
    - [Pub] transferOwnership #
       - modifiers: onlyOwner
    - [Int] _transferOwnership #

 + [Int] IERC20 
    - [Ext] totalSupply
    - [Ext] balanceOf
    - [Ext] transfer #
    - [Ext] allowance
    - [Ext] approve #
    - [Ext] transferFrom #

 + [Int] IERC165 
    - [Ext] supportsInterface

 + [Int] IERC721 (IERC165)
    - [Ext] balanceOf
    - [Ext] ownerOf
    - [Ext] safeTransferFrom #
    - [Ext] safeTransferFrom #
    - [Ext] transferFrom #
    - [Ext] approve #
    - [Ext] setApprovalForAll #
    - [Ext] getApproved
    - [Ext] isApprovedForAll

 +  PhenixMultiSigFactory (Ownable)
    - [Pub]  #
    - [Int] _setTypeFee #
    - [Ext] setTypeFee #
       - modifiers: onlyOwner
    - [Ext] setRouterAddress #
       - modifiers: onlyOwner
    - [Ext] setUSDCPairAddress #
       - modifiers: onlyOwner
    - [Int] _setAdminAddress #
    - [Ext] setAdminAddress #
       - modifiers: onlyOwner
    - [Ext] setERC721DiscountFee #
       - modifiers: onlyOwner
    - [Ext] setPayableTokenAddress #
       - modifiers: onlyOwner
    - [Ext] setIsEnabled #
       - modifiers: onlyOwner
    - [Ext] getPayableTokenBalance
    - [Ext] getBalance
    - [Pub] getMultiSigWalletType
    - [Ext] setMultiSigWalletType #
       - modifiers: isValidType,onlyOwner
    - [Ext] getMultiSigWalletInfo
    - [Pub] getOwnersOfContract
    - [Pub] getContractsOfOwner
    - [Ext] getDeployedContracts
    - [Ext] numberOfDeployedContracts
    - [Ext] takeETHFees #
       - modifiers: onlyOwner
    - [Ext] takeTokenFees #
       - modifiers: onlyOwner
    - [Pub] userCost
    - [Ext] getFeeAmountsOfType
    - [Pub] getFeeETHAmount
    - [Pub] getFeeTokenAmount
    - [Ext] generateMultiSigWalletWithETH ($)
       - modifiers: canGenerateMultiSigWallet,isValidType
    - [Ext] generateMultiSigWalletWithTokens #
       - modifiers: canPayTokenFee,isValidType,canGenerateMultiSigWallet
    - [Int] _generateMultiSigWallet #

    About SourceHat

    SourceHat has quickly grown to have one of the most experienced and well-equipped smart contract auditing teams in the industry. Our team has conducted 1300+ solidity smart contract audits covering all major project types and protocols, securing a total of over $50 billion U.S. dollars in on-chain value across 1500 projects!.
    Our firm is well-reputed in the community and is trusted as a top smart contract auditing company for the review of solidity code, no matter how complex. Our team of experienced solidity smart contract auditors performs audits for tokens, NFTs, crowdsales, marketplaces, gambling games, financial protocols, and more!

    Contact us today to get a free quote for a smart contract audit of your project!

    What is a SourceHat Audit?

    Typically, a smart contract audit is a comprehensive review process designed to discover logical errors, security vulnerabilities, and optimization opportunities within code. A SourceHat Audit takes this a step further by verifying economic logic to ensure the stability of smart contracts and highlighting privileged functionality to create a report that is easy to understand for developers and community members alike.

    How Do I Interpret the Findings?

    Each of our Findings will be labeled with a Severity level. We always recommend the team resolve High, Medium, and Low severity findings prior to deploying the code to the mainnet. Here is a breakdown on what each Severity level means for the project:

    • High severity indicates that the issue puts a large number of users' funds at risk and has a high probability of exploitation, or the smart contract contains serious logical issues which can prevent the code from operating as intended.
    • Medium severity issues are those which place at least some users' funds at risk and has a medium to high probability of exploitation.
    • Low severity issues have a relatively minor risk association; these issues have a low probability of occurring or may have a minimal impact.
    • Informational issues pose no immediate risk, but inform the project team of opportunities for gas optimizations and following smart contract security best practices.