Commit 46ebc2de authored by James Kim's avatar James Kim

add timestamp check to the invite commitments

parent e83176ba
...@@ -83,6 +83,10 @@ contract OptimistInviter_Initializer is Test { ...@@ -83,6 +83,10 @@ contract OptimistInviter_Initializer is Test {
optimistInviterHelper = new OptimistInviterHelper(optimistInviter, "OptimistInviter"); optimistInviterHelper = new OptimistInviterHelper(optimistInviter, "OptimistInviter");
} }
function _passMinCommitmentPeriod() internal {
vm.warp(optimistInviter.MIN_COMMITMENT_PERIOD() + block.timestamp);
}
/** /**
* @notice Returns a user's current invite count, as stored in the AttestationStation. * @notice Returns a user's current invite count, as stored in the AttestationStation.
*/ */
...@@ -174,7 +178,7 @@ contract OptimistInviter_Initializer is Test { ...@@ -174,7 +178,7 @@ contract OptimistInviter_Initializer is Test {
optimistInviter.commitInvite(hashedSignature); optimistInviter.commitInvite(hashedSignature);
// Check that the commitment was stored correctly // Check that the commitment was stored correctly
assertTrue(optimistInviter.commitments(hashedSignature)); assertEq(optimistInviter.commitmentTimestamps(hashedSignature), block.timestamp);
} }
/** /**
...@@ -196,7 +200,12 @@ contract OptimistInviter_Initializer is Test { ...@@ -196,7 +200,12 @@ contract OptimistInviter_Initializer is Test {
_commitInviteAs(_claimer, signature); _commitInviteAs(_claimer, signature);
// The hash(claimer ++ signature) should be committed // The hash(claimer ++ signature) should be committed
assertEq(optimistInviter.commitments(keccak256(abi.encode(_claimer, signature))), true); assertEq(
optimistInviter.commitmentTimestamps(keccak256(abi.encode(_claimer, signature))),
block.timestamp
);
_passMinCommitmentPeriod();
// OptimistInviter should issue a new attestation allowing claimer to mint // OptimistInviter should issue a new attestation allowing claimer to mint
vm.expectEmit(true, true, true, true, address(attestationStation)); vm.expectEmit(true, true, true, true, address(attestationStation));
...@@ -324,7 +333,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer { ...@@ -324,7 +333,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer {
bytes32 hashedSignature = keccak256(abi.encode(sally, signature)); bytes32 hashedSignature = keccak256(abi.encode(sally, signature));
optimistInviter.commitInvite(hashedSignature); optimistInviter.commitInvite(hashedSignature);
assertTrue(optimistInviter.commitments(hashedSignature)); assertEq(optimistInviter.commitmentTimestamps(hashedSignature), block.timestamp);
} }
/** /**
...@@ -338,7 +347,24 @@ contract OptimistInviterTest is OptimistInviter_Initializer { ...@@ -338,7 +347,24 @@ contract OptimistInviterTest is OptimistInviter_Initializer {
bytes32 hashedSignature = keccak256(abi.encode(eve, signature)); bytes32 hashedSignature = keccak256(abi.encode(eve, signature));
optimistInviter.commitInvite(hashedSignature); optimistInviter.commitInvite(hashedSignature);
assertTrue(optimistInviter.commitments(hashedSignature)); assertEq(optimistInviter.commitmentTimestamps(hashedSignature), block.timestamp);
}
/**
* @notice Attempting to commit the same hash twice should revert. This prevents griefing.
*/
function test_commitInvite_committingSameHashTwice_reverts() external {
_grantInvitesTo(bob);
(, bytes memory signature) = _issueInviteAs(bobPrivateKey);
vm.prank(sally);
bytes32 hashedSignature = keccak256(abi.encode(eve, signature));
optimistInviter.commitInvite(hashedSignature);
assertEq(optimistInviter.commitmentTimestamps(hashedSignature), block.timestamp);
vm.expectRevert("OptimistInviter: commitment already made");
optimistInviter.commitInvite(hashedSignature);
} }
/** /**
...@@ -360,10 +386,9 @@ contract OptimistInviterTest is OptimistInviter_Initializer { ...@@ -360,10 +386,9 @@ contract OptimistInviterTest is OptimistInviter_Initializer {
bytes memory signature bytes memory signature
) = _issueInviteAs(bobPrivateKey); ) = _issueInviteAs(bobPrivateKey);
_commitInviteAs(sally, signature);
vm.prank(ted); vm.prank(ted);
optimistInviter.commitInvite(keccak256(abi.encode(sally, signature))); optimistInviter.commitInvite(keccak256(abi.encode(sally, signature)));
_passMinCommitmentPeriod();
vm.expectEmit(true, true, true, true, address(attestationStation)); vm.expectEmit(true, true, true, true, address(attestationStation));
emit AttestationCreated( emit AttestationCreated(
...@@ -385,6 +410,23 @@ contract OptimistInviterTest is OptimistInviter_Initializer { ...@@ -385,6 +410,23 @@ contract OptimistInviterTest is OptimistInviter_Initializer {
assertFalse(_hasMintAttestation(eve)); assertFalse(_hasMintAttestation(eve));
} }
function test_claimInvite_claimBeforeMinCommitmentPeriod_reverts() external {
_grantInvitesTo(bob);
(
OptimistInviter.ClaimableInvite memory claimableInvite,
bytes memory signature
) = _issueInviteAs(bobPrivateKey);
_commitInviteAs(sally, signature);
// Some time passes, but not enough to meet the minimum commitment period
vm.warp(block.timestamp + 10);
vm.expectRevert("OptimistInviter: minimum commitment period has not elapsed yet");
vm.prank(sally);
optimistInviter.claimInvite(sally, claimableInvite, signature);
}
/** /**
* @notice Signature issued for previous versions of the contract should fail. * @notice Signature issued for previous versions of the contract should fail.
*/ */
...@@ -402,6 +444,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer { ...@@ -402,6 +444,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer {
); );
_commitInviteAs(sally, signature); _commitInviteAs(sally, signature);
_passMinCommitmentPeriod();
vm.expectRevert("OptimistInviter: invalid signature"); vm.expectRevert("OptimistInviter: invalid signature");
vm.prank(sally); vm.prank(sally);
...@@ -426,6 +469,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer { ...@@ -426,6 +469,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer {
); );
_commitInviteAs(sally, signature); _commitInviteAs(sally, signature);
_passMinCommitmentPeriod();
vm.expectRevert("OptimistInviter: invalid signature"); vm.expectRevert("OptimistInviter: invalid signature");
vm.prank(sally); vm.prank(sally);
...@@ -450,6 +494,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer { ...@@ -450,6 +494,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer {
); );
_commitInviteAs(sally, signature); _commitInviteAs(sally, signature);
_passMinCommitmentPeriod();
vm.expectRevert("OptimistInviter: invalid signature"); vm.expectRevert("OptimistInviter: invalid signature");
vm.prank(sally); vm.prank(sally);
...@@ -468,13 +513,14 @@ contract OptimistInviterTest is OptimistInviter_Initializer { ...@@ -468,13 +513,14 @@ contract OptimistInviterTest is OptimistInviter_Initializer {
) = _issueThenClaimShouldSucceed(bobPrivateKey, sally); ) = _issueThenClaimShouldSucceed(bobPrivateKey, sally);
// Sally tries to claim the invite using the same signature // Sally tries to claim the invite using the same signature
_commitInviteAs(sally, signature);
vm.expectRevert("OptimistInviter: nonce has already been used"); vm.expectRevert("OptimistInviter: nonce has already been used");
vm.prank(sally); vm.prank(sally);
optimistInviter.claimInvite(sally, claimableInvite, signature); optimistInviter.claimInvite(sally, claimableInvite, signature);
// Carol tries to claim the invite using the same signature // Carol tries to claim the invite using the same signature
_commitInviteAs(carol, signature); _commitInviteAs(carol, signature);
_passMinCommitmentPeriod();
vm.expectRevert("OptimistInviter: nonce has already been used"); vm.expectRevert("OptimistInviter: nonce has already been used");
vm.prank(carol); vm.prank(carol);
optimistInviter.claimInvite(carol, claimableInvite, signature); optimistInviter.claimInvite(carol, claimableInvite, signature);
...@@ -498,6 +544,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer { ...@@ -498,6 +544,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer {
// Sally tries to claim the invite // Sally tries to claim the invite
_commitInviteAs(sally, signature); _commitInviteAs(sally, signature);
_passMinCommitmentPeriod();
vm.expectEmit(true, true, true, true, address(attestationStation)); vm.expectEmit(true, true, true, true, address(attestationStation));
emit AttestationCreated( emit AttestationCreated(
...@@ -543,6 +590,8 @@ contract OptimistInviterTest is OptimistInviter_Initializer { ...@@ -543,6 +590,8 @@ contract OptimistInviterTest is OptimistInviter_Initializer {
_commitInviteAs(sally, bobSignature); _commitInviteAs(sally, bobSignature);
_commitInviteAs(sally, carolSignature); _commitInviteAs(sally, carolSignature);
_passMinCommitmentPeriod();
vm.expectRevert("OptimistInviter: invalid signature"); vm.expectRevert("OptimistInviter: invalid signature");
vm.prank(sally); vm.prank(sally);
optimistInviter.claimInvite(sally, bobClaimableInvite, carolSignature); optimistInviter.claimInvite(sally, bobClaimableInvite, carolSignature);
...@@ -560,6 +609,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer { ...@@ -560,6 +609,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer {
) = _issueInviteAs(bobPrivateKey); ) = _issueInviteAs(bobPrivateKey);
_commitInviteAs(sally, signature); _commitInviteAs(sally, signature);
_passMinCommitmentPeriod();
vm.expectRevert("OptimistInviter: issuer has no invites"); vm.expectRevert("OptimistInviter: issuer has no invites");
vm.prank(sally); vm.prank(sally);
...@@ -588,6 +638,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer { ...@@ -588,6 +638,7 @@ contract OptimistInviterTest is OptimistInviter_Initializer {
) = _issueInviteAs(bobPrivateKey); ) = _issueInviteAs(bobPrivateKey);
_commitInviteAs(eve, signature4); _commitInviteAs(eve, signature4);
_passMinCommitmentPeriod();
vm.expectRevert("OptimistInviter: issuer has no invites"); vm.expectRevert("OptimistInviter: issuer has no invites");
vm.prank(eve); vm.prank(eve);
......
...@@ -10,16 +10,33 @@ import { OptimistInviter } from "../../universal/op-nft/OptimistInviter.sol"; ...@@ -10,16 +10,33 @@ import { OptimistInviter } from "../../universal/op-nft/OptimistInviter.sol";
* Made this a separate contract instead of including in OptimistInviter.t.sol for reusability. * Made this a separate contract instead of including in OptimistInviter.t.sol for reusability.
*/ */
contract OptimistInviterHelper { contract OptimistInviterHelper {
/**
* @notice EIP712 typehash for the ClaimableInvite type.
*/
bytes32 public constant CLAIMABLE_INVITE_TYPEHASH = bytes32 public constant CLAIMABLE_INVITE_TYPEHASH =
keccak256("ClaimableInvite(address issuer,bytes32 nonce)"); keccak256("ClaimableInvite(address issuer,bytes32 nonce)");
/**
* @notice EIP712 typehash for the EIP712Domain type that is included as part of the signature.
*/
bytes32 public constant EIP712_DOMAIN_TYPEHASH = bytes32 public constant EIP712_DOMAIN_TYPEHASH =
keccak256( keccak256(
"EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)" "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
); );
/**
* @notice Address of OptimistInviter contract we are testing.
*/
OptimistInviter public optimistInviter; OptimistInviter public optimistInviter;
/**
* @notice OptimistInviter contract name. Used to construct the EIP-712 domain.
*/
string public name; string public name;
/**
* @notice Keeps track of current nonce to generate new nonces for each invite.
*/
uint256 public currentNonce; uint256 public currentNonce;
constructor(OptimistInviter _optimistInviter, string memory _name) { constructor(OptimistInviter _optimistInviter, string memory _name) {
...@@ -29,7 +46,11 @@ contract OptimistInviterHelper { ...@@ -29,7 +46,11 @@ contract OptimistInviterHelper {
} }
/** /**
* @notice Returns the hash of the struct ClaimableInvite * @notice Returns the hash of the struct ClaimableInvite.
*
* @param _claimableInvite ClaimableInvite struct to hash.
*
* @return EIP-712 typed struct hash.
*/ */
function getClaimableInviteStructHash(OptimistInviter.ClaimableInvite memory _claimableInvite) function getClaimableInviteStructHash(OptimistInviter.ClaimableInvite memory _claimableInvite)
public public
...@@ -49,13 +70,19 @@ contract OptimistInviterHelper { ...@@ -49,13 +70,19 @@ contract OptimistInviterHelper {
/** /**
* @notice Returns a bytes32 nonce that should change everytime. In practice, people should use * @notice Returns a bytes32 nonce that should change everytime. In practice, people should use
* pseudorandom nonces. * pseudorandom nonces.
*
* @return Nonce that should be used as part of ClaimableInvite.
*/ */
function consumeNonce() public returns (bytes32) { function consumeNonce() public returns (bytes32) {
return bytes32(keccak256(abi.encode(currentNonce++))); return bytes32(keccak256(abi.encode(currentNonce++)));
} }
/** /**
* @notice Returns a ClaimableInvite with the issuer and current nonce * @notice Returns a ClaimableInvite with the issuer and current nonce.
*
* @param _issuer Issuer to include in the ClaimableInvite.
*
* @return ClaimableInvite that can be hashed & signed.
*/ */
function getClaimableInviteWithNewNonce(address _issuer) function getClaimableInviteWithNewNonce(address _issuer)
public public
...@@ -66,6 +93,10 @@ contract OptimistInviterHelper { ...@@ -66,6 +93,10 @@ contract OptimistInviterHelper {
/** /**
* @notice Computes the EIP712 digest with default correct parameters. * @notice Computes the EIP712 digest with default correct parameters.
*
* @param _claimableInvite ClaimableInvite struct to hash.
*
* @return EIP-712 compatible digest.
*/ */
function getDigest(OptimistInviter.ClaimableInvite calldata _claimableInvite) function getDigest(OptimistInviter.ClaimableInvite calldata _claimableInvite)
public public
...@@ -85,6 +116,14 @@ contract OptimistInviterHelper { ...@@ -85,6 +116,14 @@ contract OptimistInviterHelper {
/** /**
* @notice Computes the EIP712 digest with the given domain parameters. * @notice Computes the EIP712 digest with the given domain parameters.
* Used for testing that different domain parameters fail. * Used for testing that different domain parameters fail.
*
* @param _claimableInvite ClaimableInvite struct to hash.
* @param _name Contract name to use in the EIP712 domain.
* @param _version Contract version to use in the EIP712 domain.
* @param _chainid Chain ID to use in the EIP712 domain.
* @param _verifyingContract Address to use in the EIP712 domain.
*
* @return EIP-712 compatible digest.
*/ */
function getDigestWithEIP712Domain( function getDigestWithEIP712Domain(
OptimistInviter.ClaimableInvite calldata _claimableInvite, OptimistInviter.ClaimableInvite calldata _claimableInvite,
......
...@@ -31,14 +31,15 @@ import { ...@@ -31,14 +31,15 @@ import {
* 5) claimer commits the hash of the address they want to receive the invite on and the * 5) claimer commits the hash of the address they want to receive the invite on and the
* received signature keccak256(abi.encode(addressToReceiveTo, receivedSignature)) * received signature keccak256(abi.encode(addressToReceiveTo, receivedSignature))
* using the commitInvite function * using the commitInvite function
* 6) claimer reveals the plaintext ClaimableInvite and the signature using the * 6) claimer waits for the MIN_COMMITMENT_PERIOD to pass.
* 7) claimer reveals the plaintext ClaimableInvite and the signature using the
* claimInvite function, receiving the "optimist.can-mint-from-invite" attestation * claimInvite function, receiving the "optimist.can-mint-from-invite" attestation
*/ */
contract OptimistInviter is Semver, EIP712Upgradeable { contract OptimistInviter is Semver, EIP712Upgradeable {
/** /**
* @notice Emitted when an invite is claimed. * @notice Emitted when an invite is claimed.
* *
* @param issuer Address that issued the signature. * @param issuer Address that issued the signature.
* @param claimer Address that claimed the invite. * @param claimer Address that claimed the invite.
*/ */
event InviteClaimed(address indexed issuer, address indexed claimer); event InviteClaimed(address indexed issuer, address indexed claimer);
...@@ -78,6 +79,18 @@ contract OptimistInviter is Semver, EIP712Upgradeable { ...@@ -78,6 +79,18 @@ contract OptimistInviter is Semver, EIP712Upgradeable {
*/ */
AttestationStation public immutable ATTESTATION_STATION; AttestationStation public immutable ATTESTATION_STATION;
/**
* @notice Minimum age of a commitment (in seconds) before it can be revealed using claimInvite.
* Currently set to 60 seconds.
*
* Prevents an attacker from front-running a commitment by taking the signature in the
* claimInvite call and quickly committing and claiming it before the the claimer's
* transaction succeeds. With this, frontrunning a commitment requires that an attacker
* be able to prevent the honest claimer's claimInvite transaction from being included
* for this long.
*/
uint256 public constant MIN_COMMITMENT_PERIOD = 60;
/** /**
* @notice Struct that represents a claimable invite that will be signed by the issuer. * @notice Struct that represents a claimable invite that will be signed by the issuer.
* *
...@@ -94,9 +107,9 @@ contract OptimistInviter is Semver, EIP712Upgradeable { ...@@ -94,9 +107,9 @@ contract OptimistInviter is Semver, EIP712Upgradeable {
} }
/** /**
* @notice Maps from hashes to whether or not they have been committed. * @notice Maps from hashes to the timestamp when they were committed.
*/ */
mapping(bytes32 => bool) public commitments; mapping(bytes32 => uint256) public commitmentTimestamps;
/** /**
* @notice Maps from addresses to nonces to whether or not they have been used. * @notice Maps from addresses to nonces to whether or not they have been used.
...@@ -127,7 +140,7 @@ contract OptimistInviter is Semver, EIP712Upgradeable { ...@@ -127,7 +140,7 @@ contract OptimistInviter is Semver, EIP712Upgradeable {
* claimed yet will no longer be accepted by the claimInvite function. Please make * claimed yet will no longer be accepted by the claimInvite function. Please make
* sure to notify the issuers that they must re-issue their invite signatures. * sure to notify the issuers that they must re-issue their invite signatures.
* *
* @param _name Contract name * @param _name Contract name.
*/ */
function initialize(string memory _name) public initializer { function initialize(string memory _name) public initializer {
__EIP712_init(_name, EIP712_VERSION); __EIP712_init(_name, EIP712_VERSION);
...@@ -181,12 +194,21 @@ contract OptimistInviter is Semver, EIP712Upgradeable { ...@@ -181,12 +194,21 @@ contract OptimistInviter is Semver, EIP712Upgradeable {
* scheme, anyone who is watching the mempool can take the signature being submitted * scheme, anyone who is watching the mempool can take the signature being submitted
* and front run the transaction to claim the invite to their own address. * and front run the transaction to claim the invite to their own address.
* *
* The same commitment can only be made once, and the function reverts if the
* commitment has already been made. This prevents griefing where a malicious party can
* prevent the original claimer from being able to claimInvite.
*
* *
* @param _commitment A hash of the claimer and signature concatenated. * @param _commitment A hash of the claimer and signature concatenated.
* keccak256(abi.encode(_claimer, _signature)) * keccak256(abi.encode(_claimer, _signature))
*/ */
function commitInvite(bytes32 _commitment) public { function commitInvite(bytes32 _commitment) public {
commitments[_commitment] = true; // Check that the commitment hasn't already been made. This prevents griefing where
// a malicious party continuously re-submits the same commitment, preventing the original
// claimer from claiming their invite by resetting the minimum commitment period.
require(commitmentTimestamps[_commitment] == 0, "OptimistInviter: commitment already made");
commitmentTimestamps[_commitment] = block.timestamp;
} }
/** /**
...@@ -196,9 +218,10 @@ contract OptimistInviter is Semver, EIP712Upgradeable { ...@@ -196,9 +218,10 @@ contract OptimistInviter is Semver, EIP712Upgradeable {
* committed using commitInvite. Before issuing the "optimist.can-mint-from-invite" * committed using commitInvite. Before issuing the "optimist.can-mint-from-invite"
* attestation, this function checks that * attestation, this function checks that
* 1) the hash corresponding to the _claimer and the _signature was committed * 1) the hash corresponding to the _claimer and the _signature was committed
* 2) the _signature is signed correctly by the issuer * 2) MIN_COMMITMENT_PERIOD has passed since the commitment was made.
* 3) the _signature hasn't already been used to claim an invite before * 3) the _signature is signed correctly by the issuer
* 4) the _signature issuer has not used up all of their invites * 4) the _signature hasn't already been used to claim an invite before
* 5) the _signature issuer has not used up all of their invites
* This function doesn't require that the _claimer is calling this function. * This function doesn't require that the _claimer is calling this function.
* *
* @param _claimer Address that will be granted the invite. * @param _claimer Address that will be granted the invite.
...@@ -210,12 +233,22 @@ contract OptimistInviter is Semver, EIP712Upgradeable { ...@@ -210,12 +233,22 @@ contract OptimistInviter is Semver, EIP712Upgradeable {
ClaimableInvite calldata _claimableInvite, ClaimableInvite calldata _claimableInvite,
bytes memory _signature bytes memory _signature
) public { ) public {
uint256 commitmentTimestamp = commitmentTimestamps[
keccak256(abi.encode(_claimer, _signature))
];
// Make sure the claimer and signature have been committed. // Make sure the claimer and signature have been committed.
require( require(
commitments[keccak256(abi.encode(_claimer, _signature))], commitmentTimestamp > 0,
"OptimistInviter: claimer and signature have not been committed yet" "OptimistInviter: claimer and signature have not been committed yet"
); );
// Check that MIN_COMMITMENT_PERIOD has passed since the commitment was made.
require(
commitmentTimestamp + MIN_COMMITMENT_PERIOD <= block.timestamp,
"OptimistInviter: minimum commitment period has not elapsed yet"
);
// Generate a EIP712 typed data hash to compare against the signature. // Generate a EIP712 typed data hash to compare against the signature.
bytes32 digest = _hashTypedDataV4( bytes32 digest = _hashTypedDataV4(
keccak256( keccak256(
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment