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

[WIP] Adds useful gadgets #56

Open
wants to merge 8 commits into
base: qol-lints
Choose a base branch
from
Open

Conversation

Ruteri
Copy link
Collaborator

@Ruteri Ruteri commented Aug 1, 2024

Gadgets:

  • VersionGadget
  • AttestationGadget
  • AuthGadget
  • CensorshipResistanceGadget
  • EncryptedInputsGadget
  • KillswitchGadget

WIP - will be adding tests and ironing out bugs

@Ruteri Ruteri changed the title Adds useful gadgets - censorship resistance, inputs and outputs encryption, versioning, authentication and authorization [WIP] Adds useful gadgets Aug 1, 2024
cr_epoch = epoch + 1;
}

// add the following modifiers to your calls to make them censorship resistant
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, delighted to see a censorship resistance queue, like Ethereum Alarm Clock but for kettle apps. This looks like a nice way to do it with a modifier. I didn't follow how it works yet though, I was expecting a queue I guess

Comment on lines 23 to 26
modifier auth_query(bytes32 role, bytes memory query, bytes memory signature) {
require(Secp256k1.verify(msg.sender, keccak256(abi.encodePacked(msg.sender, query)), signature));
require(hasRole(role, msg.sender));
_;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure what the purpose of this modifier but it can use the auth modifier internally since it is doing the same check there too.

Suggested change
modifier auth_query(bytes32 role, bytes memory query, bytes memory signature) {
require(Secp256k1.verify(msg.sender, keccak256(abi.encodePacked(msg.sender, query)), signature));
require(hasRole(role, msg.sender));
_;
modifier auth_query(bytes32 role, bytes memory query, bytes memory signature) {
auth(role);
require(Secp256k1.verify(msg.sender, keccak256(abi.encodePacked(msg.sender, query)), signature));
_;

Comment on lines 30 to 33
function auth_query_hash(bytes32 role, bytes memory query) public returns (bytes32) {
require(hasRole(role, msg.sender));
return keccak256(abi.encodePacked(msg.sender, query));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use the auth modifier from above here too?

Suggested change
function auth_query_hash(bytes32 role, bytes memory query) public returns (bytes32) {
require(hasRole(role, msg.sender));
return keccak256(abi.encodePacked(msg.sender, query));
}
function auth_query_hash(bytes32 role, bytes memory query) public auth(role) returns (bytes32) {
return keccak256(abi.encodePacked(msg.sender, query));
}

import {IAndromeda} from "src/IAndromeda.sol";

abstract contract CensorshipResistanceGadget is AttestationGadget {
function Suave() internal virtual returns (IAndromeda);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this function?

To force the contract using the CRgadget to have IAndromeda instance as part of it?

mapping(bytes32 => bool) private cr_challenges;

function offchain_restart_challenge() public returns (bytes32 challenge, bytes memory attestation) {
require(!cr_challenges[challenge]); /* already challenged */
Copy link
Contributor

Choose a reason for hiding this comment

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

this check seems off a bit.
It is checking if the returned challenge was already done but it has not been set yet. So this require will always pass because challenge at this point is still empty and not set. right? or Did I oversee something?

Comment on lines 17 to 18
challenge = Suave().localRandom();
_set_local_challenge(challenge);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the probability is very low but here the randomly generated challenge might collide with a previously generated one from before. So probably there should be a check here before setting the new local challenge.
Or do you need to generate a one shot challenge?

Suggested change
challenge = Suave().localRandom();
_set_local_challenge(challenge);
challenge = Suave().localRandom();
while(cr_challenges[challenge]) {
challenge = Suave().localRandom();
}
_set_local_challenge(challenge);

Comment on lines 61 to 73
modifier force_cr(uint256 epoch) {
/* IMPORTANT! This is set regardless of revert status */
require(cr_challenges[_get_local_challenge()]);
require(epoch >= cr_epoch); // resubmission, ignore - user's view is outdated
uint256 c_local_epoch = _get_local_epoch();
if (epoch > c_local_epoch) {
// user requested epoch higher than kettle's local view - this means noone else did so before (can be benign)
_set_local_epoch(epoch); // refuse to serve anything less than epoch in subsequent calls
c_local_epoch = epoch; // use max(local, epoch) in the rest of the function
}
require(cr_epoch >= c_local_epoch); // local view of the chain must be at least until local epoch - otherwise the chain is being censored
require(cr_epoch == epoch); // and request and chain epochs must match - otherwise we are getting spoofed
_;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused and not sure what to get out of this.
Could you please elaborate more on what this is potentially doing?
I can't see how this is censorship resistant and can you give me an example of a censorship case here where this would be prevented by this modifier?

Comment on lines 16 to 18

function all_versions() public returns (bytes32[] memory) { /* TODO */ }
/* TODO: allow using past versions, maybe */
Copy link
Contributor

Choose a reason for hiding this comment

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

Since sets data structure are not there by default, we can achieve this by using mapping and arrays.
Here is a minimal suggestion

Suggested change
function all_versions() public returns (bytes32[] memory) { /* TODO */ }
/* TODO: allow using past versions, maybe */
bytes32[] private versions;
mapping(bytes32 => bool) private versionExists;
// Adds the current version to the version history if it doesn't already exist
function add_current_version() public {
bytes32 version = current_version();
if (!versionExists[version]) {
versions.push(version);
versionExists[version] = true;
}
}
// Returns an array of all recorded versions
function all_versions() public view returns (bytes32[] memory) {
return versions;
}

abstract contract EncryptedInputsGadget is VersionGadget, AttestationGadget, CensorshipResistanceGadget {
function km() internal pure virtual returns (KeyManager);

bytes private constant input_enc_derivation_path_prefix = "m/1'";
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit

Suggested change
bytes private constant input_enc_derivation_path_prefix = "m/1'";
bytes private constant INPUT_ENC_DERIVATION_PATH_PREFIX = "m/1'";

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.

3 participants