Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(contracts-rfq): Token Zap [SLT-389] #3352
base: master
Are you sure you want to change the base?
feat(contracts-rfq): Token Zap [SLT-389] #3352
Changes from 13 commits
7ad4444
b7858bf
9265257
38d8673
61eb61d
217859c
14cef36
5ca648c
7f4c1c4
596c60d
3202b70
e76174f
ae4fe60
1ec3df8
cbc4fbd
ad8d479
2e5f7d1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid unlimited approvals to reduce security risk
Setting an unlimited allowance (
type(uint256).max
) to thetarget
contract can be risky if thetarget
is malicious or becomes compromised. It's safer to approve only the necessaryamount
.Apply this diff to approve only the required amount:
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding reentrancy protection to the
zap
functionThe
zap
function allows external calls to arbitrary contracts viafunctionCallWithValue
. To prevent potential reentrancy attacks, it's advisable to add reentrancy protection using OpenZeppelin'sReentrancyGuard
.Apply this diff to add reentrancy protection:
🧰 Tools
🪛 GitHub Check: Slither
[warning] 28-48: Unused return
TokenZapV1.zap(address,uint256,bytes) (contracts/zaps/TokenZapV1.sol#28-48) ignores return value by Address.functionCallWithValue({target:target,data:payload,value:msg.value}) (contracts/zaps/TokenZapV1.sol#46)
🛠️ Refactor suggestion
Emit an event after successful zap execution
Emitting an event upon successful execution of the
zap
function can improve transparency and facilitate off-chain tracking and debugging.Apply this diff to add an event and emit it:
🧰 Tools
🪛 GitHub Check: Slither
[warning] 28-48: Unused return
TokenZapV1.zap(address,uint256,bytes) (contracts/zaps/TokenZapV1.sol#28-48) ignores return value by Address.functionCallWithValue({target:target,data:payload,value:msg.value}) (contracts/zaps/TokenZapV1.sol#46)