code-423n4 / 2022-06-infinity-findings

4 stars 0 forks source link

Centralization Risk with onlyOwner modifier #272

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L1220

Vulnerability details

Impact

During the code review, It has been observed the all currency tokens can be withdraw by owner without timelock. The currency token should not be withdrawn by owner. This poses centralization risk.

Proof of Concept

  1. Navigate to the following contract function.
  function rescueTokens(
    address destination,
    address currency,
    uint256 amount
  ) external onlyOwner {
    IERC20(currency).safeTransfer(destination, amount);
  }

Tools Used

Code Review

Recommended Mitigation Steps

We advise the client to carefully manage the onlyOwner account private key to avoid any potential risks of being hacked. In general, we strongly recommend centralized privileges or roles in the protocol to be improved via a decentralized mechanism or smart-contract-based accounts with enhanced security practices, e.g., Multisignature wallets.

HardlyDifficult commented 2 years ago

I don't believe the exchange ever intentionally escrows ERC20 tokens. Closing this as invalid.