User assets frozen indefinetely due to lack of function to rescue preapproved tokens that turn out to be faulty

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L379

Vulnerability details

Impact

The recoverERC20 function in the PrelaunchPoints contract, which is intended to allow the owner to recover ERC20 tokens mistakenly sent to the contract, has a logical error in its token validation check. This could lead to a situation where the owner is unable to recover locked tokens that are later found to be faulty or malicious, potentially resulting in significant financial losses for users.

Issue Description

The recoverERC20 function is meant to provide a way for the contract owner to recover ERC20 tokens that were mistakenly sent to the PrelaunchPoints contract.

https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L379

function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyAuthorized {
    if (tokenAddress == address(lpETH) || isTokenAllowed[tokenAddress]) {
        revert NotValidToken();
    }
    IERC20(tokenAddress).safeTransfer(owner, tokenAmount);
    emit Recovered(tokenAddress, tokenAmount);
}

The function first checks if the tokenAddress is either the lpETH token or one of the allowed tokens (i.e., tokens that can be locked in the contract, as tracked by the isTokenAllowed mapping). If the token is lpETH or allowed, the function reverts with a NotValidToken error.

The issue is that if a token was previously allowed for locking via the allowToken function, but later turns out to be faulty or malicious, the owner will not be able to use recoverERC20 to retrieve and return these tokens to their rightful owners. The faulty tokens will remain stuck in the contract.

This could lead to a significant loss of funds for users who locked the problematic tokens, and could damage the reputation and trustworthiness of the PrelaunchPoints platform.

Proof of Concept

Consider a scenario where the owner allows a token FaultyToken that seems safe at first:

FaultyToken faultyToken = new FaultyToken();
prelaunchPoints.allowToken(address(faultyToken));

Several users lock FaultyToken in the PrelaunchPoints contract:

prelaunchPoints.lock(address(faultyToken), 1000, bytes32(0));

Later, it's discovered that FaultyToken has a critical bug that makes it non-transferable.

The owner tries to use recoverERC20 to retrieve the FaultyToken and return them to their owners:

prelaunchPoints.recoverERC20(address(faultyToken), 1000);

However, because FaultyToken is in the isTokenAllowed mapping, the recoverERC20 function reverts with NotValidToken.

The owner is unable to recover the faulty tokens, and they remain stuck in the contract, effectively causing users to lose their funds.

Tools Used

Manual code review and analysis.

Recommended Mitigation Steps

The function should handle all cases, regardless of whether the token was previously allowed or not. Replace the recoverERC20 function with the following code:

function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyAuthorized {
    if (tokenAddress == address(lpETH)) {
        revert InvalidToken();
    }

    if (isTokenAllowed[tokenAddress]) {
        isTokenAllowed[tokenAddress] = false;
    }

    IERC20(tokenAddress).safeTransfer(owner, tokenAmount);
    emit Recovered(tokenAddress, tokenAmount);
}

This will allow the owner to recover any mistakenly sent ERC20 tokens, regardless of whether they were previously allowed for locking or not. If a previously allowed token is recovered, it will be automatically disallowed to prevent further locking.

Assessed type

Other

[0xd4n1el]

This solution also allows a malicious owner to steal locked tokens. The list of provided tokens is safe, and adding a faulty token is extremely unlikely

[c4-judge]

koolexcrypto marked the issue as primary issue

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by koolexcrypto

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Invalid