Cyfrin / aderyn

Aderyn 🦜 Rust-based Solidity AST analyzer.
GNU General Public License v2.0
325 stars 46 forks source link

Unsafe ERC20 Operations should not be used in low level calls #548

Open RensR opened 3 weeks ago

RensR commented 3 weeks ago

Unsafe ERC20 Operations should not be used

To Reproduce Steps to reproduce the behavior:

Report states

Found in src/v0.8/ccip/offRamp/EVM2EVMOffRamp.sol Line: 638

      abi.encodeWithSelector(IERC20.transfer.selector, receiver, localAmount),

But the call is actually handled in a similar (but not identical) way to safeERC20.

    (success, returnData,) = CallWithExactGas._callWithExactGasSafeReturnData(
      abi.encodeWithSelector(IERC20.transfer.selector, receiver, localAmount),
      localToken,
      s_dynamicConfig.maxTokenTransferGas,
      Internal.GAS_FOR_CALL_EXACT_CHECK,
      Internal.MAX_RET_BYTES
    );

Not sure if this is a true false negative, but I could see a case being made to not trigger on cases like this

alexroan commented 3 weeks ago

At first glance is looks like IERC20.transfer.selector is being caught by the detector.

What do you think the conditions should be so that it won't catch this? Maybe when .selector is used? But then again, I can imagine scenarios where that is using an unsafe ERC20 op.

TilakMaddy commented 3 weeks ago

_callWithExactGasSafeReturnData

Is it a good idea to look for the word safe in the name of parent function call?

(if present) : Here it is _callWithExactGasSafeReturnData ?

RensR commented 3 weeks ago

I think it's impossible to perfectly solve, but maybe only trigger when the selector is used directly in a raw call and now when passed into a function. Right now it triggers when I pass a selector, which doesn't even have to be called.