0xSmartContract - Missing ReEntrancy Guard to `executeSwapDirect` function

[sherlock-admin]

0xSmartContract

high

Missing ReEntrancy Guard to executeSwapDirect function

Summary

There is no re-entry risk on true ERC-20 tokens that work according to the spec (i.e. audited, etc.).

However you can write a malicious ERC-20 with custom safetransferFrom() or approve() that have re-entrancy hooks to attack a target.

Furthermore ERC-777 is backwards compatible token standard with ERC-20 standard. ERC-777 has better usability, but it has transfer hooks that can cause re-entrancy.

Vulnerability Detail

openzeppelin's view on the reentrancy problem

ERC20 generally doesn't result in reentrancy, however ERC777 tokens can and they can maskerade as ERC20. So if a contract interacts with unknown ERC20 tokens it is better to be safe and consider that transfers can create reentrancy problems.

Impact

Although reentrancy attack is considered quite old over the past two years, there have been cases such as:

• Uniswap/LendfMe hacks (2020) ($25 mln, attacked by a hacker using a reentrancy)

• The BurgerSwap hack (May 2021) ( $7.2 million because of a fake token contract and a reentrancy exploit.)

• The SURGEBNB hack (August 2021) ($4 million seems to be a reentrancy-based price manipulation attack.)

• CREAM FINANCE hack (August 2021) ($18.8 million, reentrancy vulnerability allowed the exploiter for the second borrow.)

• Siren protocol hack (September 2021) ($3.5 million, AMM pools were exploited through reentrancy attack.)

Type of Reentrancy

Details 1 - Single Function Reentrancy 2 - Cross-Function Reentrancy 3 - Cross-Contract Reentrancy

Code Snippet

Must be re-entrancy guard to below functions

cardtopup_contract/contracts/ExchangeProxy.sol:
   90       */
   91:     function executeSwap(
   92:         address _tokenFrom,
   93:         address _tokenTo,
   94:         uint256 _amount,
   95:         bytes memory _data
   96:     ) public payable override returns (uint256) {
   97:         // native token doesn't need to be transferred explicitly, it's in tx.value
   98:         if (_tokenFrom != ETH_TOKEN_ADDRESS) {
   99:             IERC20(_tokenFrom).safeTransferFrom(msg.sender, address(this), _amount);
  100:         }
  101:         // after token is transferred to this contract, call actual swap
  102:         return executeSwapDirect(msg.sender, _tokenFrom, _tokenTo, _amount, 0, _data);
  103:     }

cardtopup_contract/contracts/ExchangeProxy.sol:
  194  
  195:         if (_tokenTo != ETH_TOKEN_ADDRESS) {
  196:             //send received tokens to beneficiary directly
  197:             IERC20(_tokenTo).safeTransfer(_beneficiary, amountReceived);
  198:         } else {
  199:             //send received eth to beneficiary directly
  200:             payable(_beneficiary).sendValue(amountReceived);
  201:             // payable(_beneficiary).transfer(amountReceived);
  202:             // should work for external wallets (currently is the case)
  203:             // but wont work for some other smart contracts due to gas stipend limit
  204          }

Duplicate of #119

Tool used

Manual Review

Recommendation

Use Openzeppelin or Solmate Re-Entrancy pattern

Here is a example of a re-entracy guard

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

contract ReEntrancyGuard {
    bool internal locked;

    modifier noReentrant() {
        require(!locked, "No re-entrancy");
        locked = true;
        _;
        locked = false;
    }
}

[McMannaman]

Missing description or use case where reentrancy can lead to other users losing funds (in that case high severity) or other user cases which have economic incentive.