Wrong implementation of `Slingshot.sol#_sendFunds()`

[code423n4]

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/Slingshot.sol#L165-L178

/// @notice Sends token funds. For native token, it unwraps wrappedNativeToken
/// @param token The address of the token to send
/// @param to The address of recipient
/// @param amount The amount of the token to send
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);
    }
}

Per the comment, this function is used for sending tokens and unwraps wrappedNativeToken when needed.

However, at L170, it first calls executioner.sendFunds() and sends tokens to the recipient from the executioner contract, and then at L171-177, it tries to send funds from the Slingshot.sol contract to the recipient again.

Based on the context, it seems that L170 should not be included in this function.

Recommendation

Change to:

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

[tommyz7]

Duplicate of #28