Description

When the user is claiming funds through the claim() function, all of the user's deposited funds are sent to the claimReceiverContract, which is set by the owner. This is a storage variable that is set using the setClaimReceiverContract() function.

Within the setClaimReceiverContract() function, the only validation done on the address of the contract is to ensure that it is not address(0). This validation is not enough, as it is possible for the owner to set the address to a contract that is not able to transfer out any ETH or ERC20 tokens that it receives. In this instance, the user's funds would be lost forever.

Impact

There is a risk that user funds may become permanently locked either by accident or as a result of deliberate actions taken by a malicious owner.

Recommendations

See Ethereum Improvement Proposal EIP-165↗ for a way to determine whether a contract implements a certain interface. This will prevent the owner from making a mistake, but it will not prevent a malicious owner from locking user funds forever.

Alternatively, consider not allowing this contract address to be modified by the owner. It should be made immutable. If the receiver contract's implementation needs to change in the future, consider using a proxy pattern to do that.

Remediation

EtherFi acknowledged this finding and stated that they understand the risk, but have mitigated it by ensuring that multiple parties are involved when setting the receiver contract. Their official response is produced below.

The receiver contract has not been set yet and will be set through multiple parties being involved with the decision, we do understand the risk however, we have mitigated this with multiple parties being involved. We do understand it only takes 1 address to make the call and this is a risk.