code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L36 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L36

Vulnerability details

Impact

EIP4337 defines introduces account abstraction. Main idea is that technically account is smart contract. And here's where the problem arises: protocol must whitelist every single user's ERC4337 account to allow interaction with protocol. Otherwise users won't be able to use protocol. This approach is impractical in reality because costs too much gas.

Proof of Concept

Now it checks whether caller is whitelisted if caller is smart contract: https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/helper/ContractWhitelist.sol#L34-L39

  // modifier is eligible sender modifier
  function _isEligibleSender() internal view {
    // the below condition checks whether the caller is a contract or not
    if (msg.sender != tx.origin)
      require(whitelistedContracts[msg.sender], "Contract must be whitelisted");
  }

And this check is performed on interactions with RdpxV2Core and PerpetualAtlanticVault

Tools Used

Manual Review

Recommended Mitigation Steps

Introduce bool isWhitelistDisabled. And don't check whitelist when isWhitelistDisabled = true It will allow to disable this check in case ERC4337 gains popularity

Assessed type

Invalid Validation

bytes032 commented 1 year ago

Leaving for judge review

c4-pre-sort commented 1 year ago

bytes032 marked the issue as sufficient quality report

bytes032 commented 1 year ago

Related to #270

c4-pre-sort commented 1 year ago

bytes032 marked the issue as low quality report

c4-judge commented 11 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)