A single point of failure is not acceptable for this project

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L6

Vulnerability details

Impact

The owner role has a single point of failure and onlyOwner can use critical a few functions.

owner role in the project:
Owner is not behind a multisig and changes are not behind a timelock.

Even if protocol admins/developers are not malicious there is still a chance for Owner keys to be stolen. In such a case, the attacker can cause serious damage to the project due to important functions. In such a case, users who have invested in project will suffer high financial losses.

8 results - 2 files



contracts/Exchange.sol:
   56:     function open() external onlyOwner {

   60:     function close() external onlyOwner {

   66:     function _authorizeUpgrade(address) internal override onlyOwner {}

  324      function setExecutionDelegate(IExecutionDelegate _executionDelegate) onlyOwner

  334:     function setPolicyManager(IPolicyManager _policyManager) onlyOwner

  342      function setOracle(address _oracle) onlyOwner

  352:     function setBlockRange(uint256 _blockRange)

contracts/Pool.sol:
  15:     function _authorizeUpgrade(address) internal override onlyOwner {}

This increases the risk of A single point of failure

See this example where a similar finding ;
code-423n4/2021-08-realitycards-findings#73

Similar vulnerability;
Private keys stolen:

Hackers have stolen cryptocurrency worth around €552 million from a blockchain project linked to the popular online game Axie Infinity, in one of the largest cryptocurrency heists on record. Security issue : PrivateKey of the project officer was stolen:
https://www.euronews.com/next/2022/03/30/blockchain-network-ronin-hit-by-552-million-crypto-heist

Tools Used

Manuel Code Review

Recommended Mitigation Steps

Add a time lock to critical functions. Admin-only functions that change critical parameters should emit events and have timelocks.

Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services.

Allow only multi-signature wallets to call the function to reduce the likelihood of an attack.

https://twitter.com/danielvf/status/1572963475101556738?s=20&t=V1kvzfJlsx-D2hfnG0OmuQ

Also detail them in documentation and NatSpec comments

[c4-judge]

berndartmueller marked the issue as duplicate of #179

[c4-judge]

berndartmueller marked the issue as satisfactory