Funds are transferred twice - Financial loss

[code423n4]

Handle

csanuragjain

Vulnerability details

Impact

This can cause financial loss as funds are transferred twice due to duplicate code used

Proof of Concept

1. Navigate to https://github.com/code-423n4/2021-10-slingshot/blob/main/contracts/Slingshot.sol
2. Observe the _sendFunds function

function _sendFunds(address token, address to, uint256 amount) internal {
        executioner.sendFunds(token, to, amount);
        if (token == nativeToken) {
            wrappedNativeToken.withdraw(amount);
            (bool success, ) = to.call{value: amount}("");
            require(success, "Slingshot: ETH Transfer failed.");
        } else {
            IERC20(token).safeTransfer(to, amount);
        }
    }

3. Observe that this function calls sendFunds function on executioner which has below definition:

function sendFunds(address token, address to, uint256 amount) external onlyOwner {
        if (token == nativeToken) {
            wrappedNativeToken.withdraw(amount);
            (bool success, ) = to.call{value: amount}("");
            require(success, "Executioner: ETH Transfer failed.");
        } else {
            IERC20(token).safeTransfer(to, amount);
        }
    }

4. As we can see

executioner.sendFunds(token, to, amount); - Makes the fund transfer on executioner.sendFunds(token, to, amount);
IERC20(token).safeTransfer(to, amount); - Transfers the fund again on line 176

Recommended Mitigation Steps

Since executioner.sendFunds(token, to, amount); is already called for performing fund transfer there is no need of further code

function _sendFunds(address token, address to, uint256 amount) internal {
        executioner.sendFunds(token, to, amount);
    }

[tommyz7]

This function should have been removed. It's internal and not used anywhere. It's invalid or non-critical.

[alcueca]

Given the function is not used anywhere, this is a non-critical issue of code quality.

[alcueca]

Duplicate of #80