`LiquidityPool` contract does not have a function that is similar to `KangarooVault.saveToken` function

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L547-L550

Vulnerability details

Impact

The KangarooVault contract has the following KangarooVault.saveToken function, which can be used to transfer any ERC20 tokens that are not sUSD from the KangarooVault. However, the LiquidityPool contract does not have this kind of function. If any ERC20 tokens that are not sUSD are transferred to the LiquidityPool contract, these tokens would be locked in the LiquidityPool contract and cannot be returned to the original senders. As a result, these senders lose such transferred ERC20 tokens.

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/KangarooVault.sol#L547-L550

    function saveToken(address token, address receiver, uint256 amt) external requiresAuth {
        require(token != address(SUSD));
        ERC20(token).transfer(receiver, amt);
    }

Proof of Concept

The following steps can occur for the described scenario.

1. Alice has sent 10000 USDC to the LiquidityPool contract.
2. The protocol team would like to return such 10000 USDC to Alice.
3. However, because the LiquidityPool contract does not have a function that is similar to the KangarooVault.saveToken function, such 10000 USDC are locked in the LiquidityPool contract and cannot be returned to Alice.
4. Alice loses such 10000 USDC.

Tools Used

VSCode

Recommended Mitigation Steps

The LiquidityPool contract can be updated to add a function that is similar to the KangarooVault.saveToken function for saving any ERC20 tokens that are not sUSD from the LiquidityPool contract.

[c4-judge]

JustDravee marked the issue as primary issue

[c4-sponsor]

mubaris marked the issue as sponsor confirmed

[c4-judge]

JustDravee marked the issue as selected for report

[huuducsc]

I believe this issue should be classified as an informational/low issue because the saveToken function doesn't have any impact on the protocol. It is only used as a safety function to recover from any mistakes in the future, and many contracts from other protocols don't have this function, since it seems unnecessary and redundant. Additionally, I don't think the scenario where users send USDC directly to the LiquidityPool is a valid one and any protocol would be at risk if they consider this case LOL. Please check it again.

P.S: I believe that the saveToken function is created to withdraw any PERP tokens that may exist in the KangarooVault contract to prevent potential attacks where an attacker sends PERP tokens to the vault after deployment to make the vault unable to open a short position. Therefore, this function is not useful for the LiquidityPool contract.

[JustDravee]

Talked about this with the sponsor. Agreed @huuducsc

[c4-judge]

JustDravee changed the severity to QA (Quality Assurance)

[c4-judge]

JustDravee marked the issue as not selected for report

[c4-judge]

JustDravee marked the issue as grade-a