code-423n4 / 2023-08-dopex-findings

3 stars 3 forks source link

Upgraded Q -> 3 from #839 [1698131435131] #2228

Closed c4-judge closed 10 months ago

c4-judge commented 10 months ago

Judge has assessed an item in Issue #839 as 3 risk. The relevant finding follows:

UniV3LiquidityAMO: recoverERC721() does not, in fact, recovers them The function to recover ERC721’s (found here) sends them to the rDPX V2 core contract, however said contract has no function to retrieve them, rendering the function useless and losing the token forever. If we take a closer look at emergencyWithdraw() shown below, we’ll see it cannot properly retrieve ERC721’s as they only function with ERC20’s.

function emergencyWithdraw( address[] calldata tokens ) external onlyRole(DEFAULT_ADMIN_ROLE) { _whenPaused(); IERC20WithBurn token;

for (uint256 i = 0; i < tokens.length; i++) {
  token = IERC20WithBurn(tokens[i]);
  token.safeTransfer(msg.sender, token.balanceOf(address(this)));
}

emit LogEmergencyWithdraw(msg.sender, tokens);

} The comment below can be found in the recovery function:

// Only the owner address can ever receive the recovery withdrawal

Which leaves me to assume this was an accident. The V2 core contract inherits ERC721Holder which only includes the onERC721Received() function. The transferring to the core contract will succeed but that’s as far as it goes. In order to fix this, consider sending this recovered asset to an owner’s EOA address instead.

c4-judge commented 10 months ago

GalloDaSballo marked the issue as duplicate of #935

c4-judge commented 10 months ago

GalloDaSballo marked the issue as satisfactory