Skip to content
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

HAL-03 lack of validation on setblacklist message #3254

Open
juniuszhou opened this issue Sep 18, 2022 · 2 comments
Open

HAL-03 lack of validation on setblacklist message #3254

juniuszhou opened this issue Sep 18, 2022 · 2 comments
Assignees
Labels
Peggy Team Peggy team task

Comments

@juniuszhou
Copy link
Contributor

Description

The SetBlacklist message does not have any validation performed on itvia the ValidateBasic() method. It is recommended to always perform validations on messages via ValidateBasic() when possible.
A lack of validation here could allow for bad, meaningless data to be stored in the chain’s state, leading to unpredictable behaviour.
An attacker could also use this feature to deny service to Ethereum addresses that provide useful functionality to users, and thus degrade the availability of software functionality.

Recommendation

It is recommended to perform validation in ValidateBasic whenever possi�ble. In this specific case, JSON is passed into the Message containing Ethereum addresses.

The following list of validations could be performed:

  1. The JSON contains only valid Ethereum addresses
  2. Addresses that are used by the application (such as the Peggy bridge contract, other contracts known to be safe, contract libraries used by Sifchain, and so on) should not be permitted in the blacklist

Note that these suggestions are non-exhaustive, and it may be appropriate to add additional validations depending on the desired functionality of the blacklist.

@juniuszhou juniuszhou self-assigned this Sep 18, 2022
@juniuszhou
Copy link
Contributor Author

#3219 add the check for ethereum address. But for specific addresses like Peggy bridge contract or other contracts we deployed in Ethereum not checked.

@juniuszhou juniuszhou added the Peggy Team Peggy team task label Sep 18, 2022
@juniuszhou juniuszhou assigned Brando753 and smartyalgo and unassigned juniuszhou Sep 29, 2022
@juniuszhou
Copy link
Contributor Author

I don't think it is necessary to add these special contract address into consideration. If @Brando753 or @smartyalgo agree with me, we can close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Peggy Team Peggy team task
Projects
None yet
Development

No branches or pull requests

3 participants