This repository has been archived by the owner on Apr 23, 2024. It is now read-only.
Address issues in smart contract from security audit #93
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tamirms
force-pushed
the
sc-audit-fixes
branch
from
0345b26
to
559b8a9
Compare
tamirms
force-pushed
the
sc-audit-fixes
branch
from
2fd0e54
to
1bf9065
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR implements fixes for "SBR-1 Replay attacks between chains" and "SBR-2 Deposited funds potentially locked in contract" from the security audit report:
https://drive.google.com/file/d/1f_IqUB3k3Q9XjLhG38zK5XWRF43tLZ_t/view
SBR1 is addressed by including a
domainSeparatorin the hash which is signed by the validators. ThedomainSeparatorconsists of the validator set version, the network chain id, and the bridge smart contract address. Including those components ensures that signatures cannot be replayed on bridge contracts deployed on multiple chains (e.g. testnet and mainnet).SBR-2 is addressed by
having a restricted list of all supported ERC20 tokens which can be deposited to the bridge contract. It is up to the validators to ensure that this allow list will contain only standard ERC20 tokens which do not subtract fees in tokens from the transferred value.[Edit: I removed the deposit token allow list and instead explicitly checked the amount of tokens transferred to the bridge as suggested by the security audit]
Note that "SBR-3 Possible name or symbol collisions for Stellar assets" is not addressed because I think the validators can agree to not reuse symbols or names when registering Stellar assets. I think this behavior can be enforced through validator behavior and I don't think it's necessary to enforce name collision prevention in the smart contract logic. If the validators were compromised then there would be attacks of greater impact than just registering stellar assets with duplicate names.