Every token accidentally sent to the Contract cannot be recovered

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L653-L661

Vulnerability details

Impact

The purpose of the recoverERC20 function is to recover the incorrectly sent tokens to the contract. However, its main purpose is restricted due to the minAmountRewardToken[token] != 0 condition. However, this issue is not mentioned in the NatSpec comments. Due to the restriction, the main task of the function is to recover users' incorrectly sent tokens, 100% cannot be performed

contracts/WardenPledge.sol:

 /**
    * @notice Recovers ERC2O tokens sent by mistake to the contract
    * @dev Recovers ERC2O tokens sent by mistake to the contract
    * @param token Address tof the EC2O token
    * @return bool: success
    */
  653:     function recoverERC20(address token) external onlyOwner returns(bool) {
  654:         if(minAmountRewardToken[token] != 0) revert Errors.CannotRecoverToken();
  655: 
  656:         uint256 amount = IERC20(token).balanceOf(address(this));
  657:         if(amount == 0) revert Errors.NullValue();
  658:         IERC20(token).safeTransfer(owner(), amount);
  659: 
  660:         return true;
  661:     }

Owner has the possibility to save the token to be recovered in the minAmountRewardToken mapping and recover later, but the risk remains due to the possibility of the owner to renounceOwnership

Marked as Medium due to risk of losing user funds.

Proof of Concept

1-Alice is the onlyOwner person of the contract and has renounceOwnership with the authority in the Ownable contract 2-Bob sends 10,000-USDT to the contract due to an incorrect copy-paste 3-This amount can never be recovered and is stuck in the contract forever

Tools Used

Manual Code Review

Recommended Mitigation Steps

Disable the renounceOwnership property, this also avoids the risk that other onlyOwner functions will not run forever. Remove the if block in the function

            function recoverERC20(address token) external onlyOwner returns(bool) {
                  uint256 amount = IERC20(token).balanceOf(address(this))
                  IERC20(token).safeTransfer(owner(), amount);
                  return true;
                }

[trust1995]

Combining renounceOwnership() likelihood with accidental sending of ERC20 sounds like Low severity. Also the Ownable.sol contract is out of scope.

[Kogaroshi]

Duplicate of #103

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-10-paladin-findings/issues/187