-
Notifications
You must be signed in to change notification settings - Fork 466
Remove SignatureType.Caller #1015
Remove SignatureType.Caller #1015
Conversation
), | ||
"INVALID_SIGNATURE" | ||
); | ||
if (signerAddress != msg.sender) { |
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.
How will this interact with executeTransaction
? Seems like currently the exchange contract can become a valid signer for anything.
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.
I think we need to add a require(msg.sender != this)
here.
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.
Not an issue since we are using DELEGATECALL
and are not otherwise making arbitrary self-calls.
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.
Would this mean that if I call executeTransaction
and then call preSign
that I could presign orders for the Exchange
contract?
I recommend to not allow adding pre signed signatures if signerAddress == msg.sender
.
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.
executeTransaction
uses delegatecall
, so the Exchange contract should never be msg.sender
.
We've basically determined that usage of msg.sender
here should be safe (this is a pretty common pattern). If we were using getCurrentContextAddress
, there would definitely be extra protections.
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 is no direct vulnerability in the 0x protocol relating to this change. A scenario where an entity creates a smart contract that has a token balance and has approved the AssetProxies could still be vulnerable if they allow forward calls to the exchange contract and the function preSign
is callable. It's acknowledged that this is outside of the control of 0x and for that reason the issue is considered closed.
I still recommend caution around the assumption that no signature validation needs to be performed when msg.sender == signerAddress
.
), | ||
"INVALID_SIGNATURE" | ||
); | ||
if (signerAddress != msg.sender) { |
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.
Not an issue since we are using DELEGATECALL
and are not otherwise making arbitrary self-calls.
951e219
to
ad0e4fe
Compare
… message signed by Trezor One (firmware v1.6.2)
1397b04
to
0629a7d
Compare
Description
Testing instructions
Types of changes
Checklist:
[WIP]
if necessary.[sol-cov] Fixed bug
.