-
Notifications
You must be signed in to change notification settings - Fork 305
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
Feature/spear bits audit3 #87
Conversation
7362d3f
to
0e86264
Compare
9a193f3
to
04e5072
Compare
04e5072
to
744ac34
Compare
@@ -153,6 +155,9 @@ contract PolygonZkEVMBridge is | |||
// Ether is treated as ether from mainnet | |||
originNetwork = MAINNET_NETWORK_ID; | |||
} else { | |||
// Check msg.value is 0 if tokens are bridged | |||
require(msg.value == 0, "PolygonZkEVMBridge::bridgeAsset: Expected zero native asset value when bridging ERC20 tokens"); |
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.
What if there is an ERC20 token that requires some msg.value
in order to perform its safeTransferFrom
custom implementation?
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.
This is not contemplated, and if it was the case we should send the msg.value along with the transferFrom function, which i think usually is not payable
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.
This avoids bad usage of the Bridge but I think it is not required xD
5ee8e99
to
745f589
Compare
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.
😸
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.
💯
This PR solves the following issues:
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/18#issuecomment-1403372039
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/14
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/16
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/17
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/18
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/20
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/21
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/25
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/26
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/30
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/31
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/33
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/36