tokamak-network / multisender_shivansh

fork for internal code review
0 stars 0 forks source link

Setting a specific gas instead of using gas() for a call() #7

Closed nguyenzung closed 4 months ago

nguyenzung commented 5 months ago

Describe the bug

Configuration

Severity: Medium ~ High Users can lost gas fee in some edge cases. It is possible but difficult for checking those manually, but I think it is better if we provide a specific gas instead of using gas() in call(). It is safer for senders to use batch transfer.

Recommendation Provide a specific gas instead of using gas() in call()

Exploit Scenario

Demo

Shivansh070707 commented 5 months ago
image

I think its better if keep using gas()

usgeeus commented 5 months ago

image I think its better if keep using gas()

@Shivansh070707 This does not answer the issue @nguyenzung raised.

Shivansh070707 commented 5 months ago

if there's any custom transfer function and we dont know the gas then also in edge case the transaction can revert if we hardcode the custom gas

nguyenzung commented 5 months ago

Thank you very much @Shivansh070707

I think it is risky for senders if there is an embedded call in ERC20.transfer(). So what do you think if we only support a simple transfer()? I think it is okay if the gas limit is fix and the transaction will be reverted. (Actually, Wallets likes Metamask will warn senders and they will not make any transactions.

zzooppii commented 4 months ago

@nguyenzung The issue was reflected. Please check it once. If the issue has been resolved, please close it.