ZdravkoHr. - `AmirX.stablecoinSwap` will always revert when `ss.origin == address(0)`

[sherlock-admin2]

ZdravkoHr.

medium

AmirX.stablecoinSwap will always revert when ss.origin == address(0)

Summary

AmirX.stablecoinSwap, when called with ss.origin == address(0), will always revert and swaps will not be possible.

Vulnerability Detail

When AmirX.stableCoinSwap gets called with ss.origin, the execution will eventually query the balance of the origin token with balanceOf(). But calling it on address(0) will revert the whole transaction and the operation will not be successfull.

Impact

DoS of some token swaps.

Code Snippet

    function stablecoinSwap(
        address wallet,
        address safe,
        StablecoinSwap memory ss,
        DefiSwap memory defi
    ) external payable onlyRole(SWAPPER_ROLE) {
        // checks if it will fail
        _verifyStablecoin(wallet, safe, ss, defi);

        //eXYZ ot eXYZ
        if (isXYZ(ss.origin) && isXYZ(ss.target)) {
            swapAndSend(wallet, ss);
            return;
        }

        //stablecoin swap
        if (isXYZ(ss.origin) && !isXYZ(ss.target))
            convertFromEXYZ(wallet, safe, ss);

        //defi swap
        uint256 iBalance = ERC20(ss.origin).balanceOf(wallet);
        if (defi.walletData.length != 0) defiSwap(wallet, safe, defi);
        uint256 fBalance = ERC20(ss.origin).balanceOf(wallet);
        // //stablecoin swap
        if (!isXYZ(ss.origin) && isXYZ(ss.target)) {
            if (fBalance - iBalance != 0) ss.oAmount = fBalance - iBalance;
            convertToEXYZ(wallet, safe, ss);
        }
    }

Tool used

Manual Review

Recommendation

Do not call balanceOf if ss.origin == address(0)

if (ss.origin != address(0)) {
            uint256 iBalance = ERC20(ss.origin).balanceOf(wallet);
            if (defi.walletData.length != 0) defiSwap(wallet, safe, defi);
            uint256 fBalance = ERC20(ss.origin).balanceOf(wallet);
            if (fBalance - iBalance != 0) ss.oAmount = fBalance - iBalance;
        } else if (defi.walletData.length != 0) defiSwap(wallet, safe, defi);

[sherlock-admin3]

2 comment(s) were left on this issue during the judging contest.

WangAudit commented:

0 address check; invalid according to rules; why would we make ss.origin = address(0)?

takarez commented:

invalid

[sherlock-admin4]

The protocol team fixed this issue in PR/commit https://github.com/telcoin/telcoin-contracts/pull/6.

[WangSecurity]

Update: after consulting with the sponsor and LSW, we decided to keep this one invalid:

1. Zero address checks are invalid due under Sherlock's rules.
2. When ss.origin = address(0) the stablecoinSwap shouldn't be used, cause ss.origin = address(0) is used for defi trades when no stablecoins are used. Therefore, in that case the defiSwap function should be called directly.

Yes, the team decided to fix it, but the issue is still invalid due to reasons addressed above.

[spacegliderrrr]

Fix looks good, address(0) check added

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.