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

Added prefix for value contract storage #260

Merged
merged 3 commits into from
Aug 7, 2023
Merged

Conversation

pierluca
Copy link
Contributor

@pierluca pierluca commented Aug 4, 2023

There's a weakness in Dela (#258) whereby the (existing) smart contracts store key=values without any prefix for the keys in a shared key/value store. Effectively this would allow any malicious user to overwrite other smart contracts' data.

This fixes it for the value contract.

We probably need to do something similar for https://github.com/dedis/d-voting

@PascalinDe @lanterno for your information, the current key/value store is limited to keys of 32 bytes. Do reach out if adding a prefix could be a problem for D-Voting (we haven't checked).

Copy link

@PascalinDe PascalinDe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pierluca
Copy link
Contributor Author

pierluca commented Aug 4, 2023

I haven't checked the CI, but I expect some integration tests will be broken.
More work is needed, but it should give a pointer to the general direction.

@sonarcloud
Copy link

sonarcloud bot commented Aug 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

11.8% 11.8% Coverage
0.4% 0.4% Duplication

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5764771956

  • 16 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 98.739%

Totals Coverage Status
Change from base Build 5764444164: 0.03%
Covered Lines: 14645
Relevant Lines: 14832

💛 - Coveralls

@PascalinDe
Copy link

I haven't checked the CI, but I expect some integration tests will be broken. More work is needed, but it should give a pointer to the general direction.

ah, I didn't check the tests since I'm used to a "the dev's responsability approach"

can you mark PR's that are not meant for a merging review as draft please? I don't mind reviewing WIP, but it gives me an idea of what to look out for 🙂

@jbsv jbsv merged commit 445cdea into extend_crypto Aug 7, 2023
7 checks passed
@jbsv jbsv deleted the value_contract_prefix branch August 7, 2023 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants