Mistakenly input `refundAddress` can lead to permanent fund loss

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-axelar/blob/9c4c44b94cddbd48b9baae30051a4e13cbe39539/contracts/deposit-service/AxelarDepositService.sol#L35-L53

Vulnerability details

Mistakenly input refundAddress can lead to permanent fund loss

Impact

If a user provided a wrong refundAddress at the first place, there is no way to get the refund, since the user won't have access to the wrong refundAddress. And no other way of refund is available. In this case, the user loss fund and not recoverable.

Deemed as medium because assets not at direct risk, but leak value with a hypothetical stated assumptions, or external requirements.

Proof of Concept

contracts/deposit-service/AxelarDepositService.sol

    function addressForTokenDeposit(
        bytes32 salt,
        address refundAddress,
        string calldata destinationChain,
        string calldata destinationAddress,
        string calldata tokenSymbol
    ) external view returns (address) {
        return
            _depositAddress(
                salt,
                abi.encodeWithSelector(
                    ReceiverImplementation.receiveAndSendToken.selector,
                    refundAddress,
                    destinationChain,
                    destinationAddress,
                    tokenSymbol
                )
            );
    }

The address for deposit is generated with refundAddress. If initially the refundAddress is mistakenly provided, and fund is deposited into the generated address, there is no way to retrieve the refund. Since according to the following, the only address refund can go is the refundAddress.

contracts/deposit-service/ReceiverImplementation.sol

        _safeTransfer(refund, refundAddress, IERC20(refund).balanceOf(address(this)));

Tools Used

Manual analysis.

Recommended Mitigation Steps

Provide alternative option for refund, in case of mistakenly input refundAddress, the refund can go back to the original payment address.

[re1ro]

Good spot. We might consider some improvements there. But the initial rationale is the same as providing wrong address to ERC20.transfer(), funds get lost.

[GalloDaSballo]

Am conflicted on this report as the "vulnerability" is equivalent to performing a transfer to the wrong address, which is not a vulnerability at all.

I think we can agree that no zero check is performed on the address, so we could downgraded to QA as "unchecked address"

In terms of knowing if the address can be used or not, I don't believe that is achievable realistically as for example any contract or multisig wallet may not be available on a separate chain

I'll downgrade to QA-LackOfChecks