Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
RensR opened this issue Jun 20, 2024 · 3 comments
Open

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

RensR opened this issue Jun 20, 2024 · 3 comments
Labels
priority-low Low priority issue

Comments

@RensR
Copy link
Contributor

RensR commented Jun 20, 2024

Unsafe ERC20 Operations should not be used

To Reproduce
Steps to reproduce the behavior:

  • Clone repo: https://github.com/smartcontractkit/ccip
  • Run aderyn with this foundry profile ccip

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
Copy link
Contributor

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
Copy link
Collaborator

TilakMaddy commented Jun 21, 2024

_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 ?

@alexroan alexroan added priority-low Low priority issue priority-medium Medium priority issue - Tackle after priority-high and removed priority-low Low priority issue priority-medium Medium priority issue - Tackle after priority-high labels Jun 21, 2024
@RensR
Copy link
Contributor Author

RensR commented Jun 25, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-low Low priority issue
Projects
None yet
Development

No branches or pull requests

3 participants