search

sherlock-audit / 2024-02-telcoin-platform-audit-update-judging

3 stars 1 forks source link

cheatcode - Unsafe use of low-level calls in AmirX::defiSwap function #77

Closed sherlock-admin4 closed 7 months ago

sherlock-admin4 commented 7 months ago

cheatcode

medium

Unsafe use of low-level calls in AmirX::defiSwap function

Summary

The use of call for sending Ether or interacting with untrusted contracts is a well-known security concern in Ethereum smart contract development. This method provides a lot of flexibility but also introduces significant risks, including reentrancy attacks and unexpected changes in contract state.

Vulnerability Detail

In the AmirX contract, particularly within the defiSwap function the code looks like this:

function defiSwap(
        address wallet,
        address safe,
        DefiSwap memory defi
    ) public payable onlyRole(SWAPPER_ROLE) {
        (bool walletResult, ) = wallet.call{value: 0}(defi.walletData);
        require(walletResult, "AmirX: wallet transaction failed");

        _feeDispersal(safe, defi);
    }

In this code:

Impact

Risks Involved with call

  1. The call method can lead to reentrancy attacks if the called contract calls back into any function of the AmirX contract, potentially leading to unexpected behaviors or vulnerabilities, especially if state changes occur after the call.

  2. By default, call forwards all available gas, potentially allowing a called contract to consume all gas provided to the function, affecting the execution of later operations.

  3. Since call is used to execute arbitrary bytecode, if the data in defi.walletData is crafted maliciously or if wallet is a malicious contract, it can lead to unintended actions being performed.

Code Snippet

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/main/telcoin-contracts/contracts/swap/AmirX.sol#L103

Tool used

Manual Review

Recommendation

Avoid using call for Ether transfers. For sending Ether, prefer using transfer or send as they limit the gas sent to 2300, preventing reentrancy attacks. For Solidity 0.6.x and above, using call for Ether transfers is recommended but with proper reentrancy guards.

sherlock-admin4 commented 7 months ago

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

WangAudit commented:

interesting; but low; 1 & 3 impact are vague; 2 is not bad; but it would dos only this exact txn and after it; the swapper has the ability to not execute it ever again; so no major DOS to be considered as a valid issue