function recoverERC20FromStaking calls recoverERC20 with wrong parameters

Summary

Function recoverERC20FromStaking calls recoverERC20 with wrong parameters

Vulnerability Detail

Function recoverERC20 expects to recieve IERC20 tokenAddress, uint256 tokenAmount, address to in this order. However the recoverERC20FromStaking function calls the recoverERC20 function with wrongly arranged parameters

Impact

The code may not function as intended

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/telx/core/StakingRewardsManager.sol#L216-L237

function recoverERC20FromStaking(
        StakingRewards staking,
        IERC20 tokenAddress,
        uint256 tokenAmount,
        address to
    ) external onlyRole(SUPPORT_ROLE) {
        // grab the tokens from the staking contract
        staking.recoverERC20(to, tokenAddress, tokenAmount);
    }

function recoverERC20(
        IERC20 tokenAddress,
        uint256 tokenAmount,
        address to
    ) external onlyRole(SUPPORT_ROLE) {
        //move funds
        tokenAddress.safeTransfer(to, tokenAmount);
    }

Tool used

Manual Review

Recommendation

Instead of staking.recoverERC20(to, tokenAddress, tokenAmount);, use staking.recoverERC20(tokenAddress, tokenAmount, to);

function recoverERC20FromStaking(
        StakingRewards staking,
        IERC20 tokenAddress,
        uint256 tokenAmount,
        address to
    ) external onlyRole(SUPPORT_ROLE) {
        // grab the tokens from the staking contract
        staking.recoverERC20(tokenAddress, tokenAmount, to);
    }

amshirif commented 6 months ago

There are two different recoverERC20() functions on two different contracts. The ordering of the two functions are different. The recoverERC20FromStaking() function is not attempting to call the recoverERC20() on the StakingRewardsManager. This function is external as can be seen by the code snippet provided. It is calling a function by the same name of the StakingRewards contract that is out of scope.