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

Fix/audit suggestions #16

Merged
merged 13 commits into from
Sep 28, 2023
Merged

Fix/audit suggestions #16

merged 13 commits into from
Sep 28, 2023

Conversation

Th0rgal
Copy link
Member

@Th0rgal Th0rgal commented Sep 27, 2023

This pull request implements the changes that were suggested during the audits and code reviews.

Big changes:

limit_price was replaced by an allowance

What it means:

Instead of having a renewing_allowance storage mapping defines as:
(renewer, domain, limit_price) -> 1 or 0
We have:
(renewer, domain) -> limit_price

Why?

  • It is actually cheaper. Even though limit_price is a uint256 made of two felts, the high part will always be zero and thus have no associated storage cost. In addition it reduces the amount of hashes required to compute the storage address.
  • It means less data to index and makes the logic easier to understand.
  • It makes sure one user can only allow one flow of payment.

Replaces limit_price(s) by domain_price(s) in _renew, renew and batch_renew

What it means:

The whitelisted renewer no longer has to specify the yearly allowance set by every user. Instead he has to specify the expected domain_price. The contract will still ensure (domain_price + tax_price) <= allowance so this doesn't change the security guarantees.

Why?

It allows us however to debit less money from users if they allowed us more than needed. As before there is no guarantee about that, but we think having that possibility could be useful to avoid getting too much dust on the contract, especially if the stark domain prices decrease and users didn't have time to update their allowance.

Small changes:

  • removed unused imports
  • fixed inconsistent sanity checks of span lengths
  • fixed incorrect comments & readme
  • added a two step process for updating the admin
  • added more events for helping user to track the contract state

closes #17

@Th0rgal Th0rgal requested a review from irisdv September 27, 2023 08:33
Copy link
Collaborator

@irisdv irisdv left a comment

Choose a reason for hiding this comment

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

lgtm!

// initialize contracts
let (erc20, pricing, starknetid, naming, autorenewal) = deploy_contracts();

// buy TH0RGAL_DOMAIN for a year
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's not the right comment

@Th0rgal Th0rgal merged commit d51027a into main Sep 28, 2023
0 of 3 checks passed
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.

Automatic renewal audit update
2 participants