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.

Sign up for GitHub

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

Jump to bottom

feat(DepositContract): Introduce operator pubkey mapping #2089

Closed
wants to merge 3 commits into from
Closed

feat(DepositContract): Introduce operator pubkey mapping #2089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2359b2
operator pubkey mapping
sj448 Oct 22, 2024
59d66d9
lint
sj448 Oct 22, 2024
60ccfdd
bet
sj448 Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion contracts/src/staking/DepositContract.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ abstract contract DepositContract is IDepositContract, ERC165 {
/// have been made to the contract.
uint64 public depositCount;

/// @dev The operator address for each pubkey.
mapping(bytes => address) private operatorByPubKey;

/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/* VIEWS */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
Expand All @@ -53,6 +56,11 @@ abstract contract DepositContract is IDepositContract, ERC165 {
|| interfaceId == type(IDepositContract).interfaceId;
}

/// @inheritdoc IDepositContract
function getOperator(bytes calldata pubkey) public view returns (address) {
return operatorByPubKey[pubkey];
}

/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/* WRITES */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
Expand All @@ -62,7 +70,8 @@ abstract contract DepositContract is IDepositContract, ERC165 {
bytes calldata pubkey,
bytes calldata credentials,
uint64 amount,
bytes calldata signature
bytes calldata signature,
address operator
Comment on lines +73 to +74
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Multiple deposit calls missing the operator parameter. Please update all invocations to include the new parameter.

🔗 Analysis chain

Verify all calls to deposit include the new operator parameter

With the addition of the operator parameter to the deposit function, ensure that all existing calls to this function throughout the codebase have been updated accordingly to prevent any breaking changes.

Run the following script to locate all invocations of deposit and check for compliance:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all calls to `deposit` and display their usage.
# Expected Result: All calls should include the new `operator` parameter.

rg -A 2 "deposit\(" --glob "contracts/**/*.sol"

Length of output: 3228

Script:

#!/bin/bash
# Description: Find all calls to `deposit` and count the number of arguments passed.
# Expected Result: All calls should have the expected number of parameters (including `operator`).

rg -n 'deposit\s*\(' --glob 'contracts/**/*.sol' | while read -r line; do
    file=$(echo "$line" | cut -d':' -f1)
    lineno=$(echo "$line" | cut -d':' -f2)
    call=$(echo "$line" | cut -d':' -f3-)
    # Extract the arguments within the parentheses
    args=$(echo "$call" | sed -n 's/.*deposit\s*(\([^)]*\)).*/\1/p')
    # Count the number of arguments by splitting on commas not within parentheses
    arg_count=$(echo "$args" | awk -F',' '{print NF}')
    echo "$file:$lineno: $arg_count arguments"
done

Length of output: 9496

)
public
payable
Expand All @@ -80,6 +89,16 @@ abstract contract DepositContract is IDepositContract, ERC165 {
revert InvalidSignatureLength();
}

// Set operator on the first deposit.
// zero `operatorByPubKey[pubkey]` means the pubkey is not registered.
if (operatorByPubKey[pubkey] == address(0)) {
if (operator == address(0)) {
revert ZeroOperatorOnFirstDeposit();
}
operatorByPubKey[pubkey] = operator;
emit OperatorUpdated(pubkey, operator, address(0));
}

uint64 amountInGwei = _deposit(amount);

if (amountInGwei < MIN_DEPOSIT_AMOUNT_IN_GWEI) {
Expand All @@ -94,6 +113,31 @@ abstract contract DepositContract is IDepositContract, ERC165 {
}
}

function updateOperator(
Copy link
Author

Choose a reason for hiding this comment

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

@neverDefined are we okay with one step operator change process?

Copy link
Contributor

Choose a reason for hiding this comment

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

two step would be good, on updates only though

bytes calldata pubkey,
address newOperator
)
external
{
// cache the current operator
address currentOperator = operatorByPubKey[pubkey];
// This will also revert if the pubkey is not registered.
if (msg.sender != currentOperator) {
revert NotCurrentOperator();
}
// Revert if the new operator is zero address.
if (newOperator == address(0)) {
revert ZeroAddress();
}
// update the operator
operatorByPubKey[pubkey] = newOperator;
emit OperatorUpdated(pubkey, newOperator, currentOperator);
}
sj448 marked this conversation as resolved.
Show resolved Hide resolved

/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/* INTERNAL */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/

/// @dev Validates the deposit amount and sends the native asset to the zero address.
function _deposit(uint64) internal virtual returns (uint64) {
if (msg.value % 1 gwei != 0) {
Expand Down
38 changes: 37 additions & 1 deletion contracts/src/staking/IDepositContract.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,16 @@ interface IDepositContract {
uint64 index
);

/**
* @notice Emitted when the operator of a validator is updated.
* @param pubkey The pubkey of the validator.
* @param newOperator The new operator address.
* @param previousOperator The previous operator address.
*/
event OperatorUpdated(
bytes indexed pubkey, address newOperator, address previousOperator
);

/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/* ERRORS */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
Expand All @@ -47,6 +57,30 @@ interface IDepositContract {
/// @dev Error thrown when the signature length is not 96 bytes.
error InvalidSignatureLength();

/// @dev Error thrown when the input operator is zero address on the first deposit.
error ZeroOperatorOnFirstDeposit();

/// @dev Error thrown when the caller is not the current operator.
error NotCurrentOperator();

/// @dev Error thrown when the address is zero address.
error ZeroAddress();

/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/* VIEWS */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/

/**
* @notice Get the operator address for a given pubkey.
* @dev Returns zero address if the pubkey is not registered.
* @param pubkey The pubkey of the validator.
* @return The operator address for the given pubkey.
*/
function getOperator(bytes calldata pubkey)
external
view
returns (address);

/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/* WRITES */
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
Expand All @@ -59,13 +93,15 @@ interface IDepositContract {
* validator operator public key, if subsequent deposit it is the depositor's public key.
* @param amount is the amount of stake native/ERC20 token to be deposited, in Gwei.
* @param signature is the signature used only on the first deposit.
* @param operator is the address of the operator.
* @dev emits the Deposit event upon successful deposit.
*/
function deposit(
bytes calldata pubkey,
bytes calldata credentials,
uint64 amount,
bytes calldata signature
bytes calldata signature,
address operator
Comment on lines +96 to +104
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Some deposit calls are missing the new operator parameter. Please update all implementations and invocations to include it.

🔗 Analysis chain

Verify all implementations and calls to deposit include the new operator parameter.

Adding the operator parameter to the deposit function in IDepositContract is a breaking change. Ensure that all contracts implementing this interface and any external calls have been updated to match the new function signature to prevent integration issues.

Run the following script to identify implementations and calls that may need updating:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all implementations and invocations of `deposit` that may lack the new `operator` parameter.

# Search for `deposit` function definitions excluding the interface file.
rg --glob '!contracts/src/staking/IDepositContract.sol' -t solidity -A 3 'function deposit\('

# Search for invocations of the `deposit` function in Solidity files.
rg -t solidity -A 1 '\.deposit\('

Length of output: 197

Script:

#!/bin/bash
# Description: Find all implementations and invocations of `deposit` that may lack the new `operator` parameter.

# Search for `deposit` function definitions excluding the interface file.
rg --glob '!contracts/src/staking/IDepositContract.sol' -e sol -A 3 'function deposit\('

# Search for invocations of the `deposit` function in Solidity files.
rg -e sol -A 1 '\.deposit\('

Length of output: 232

Script:

#!/bin/bash
# Description: Find all implementations and invocations of `deposit` in Solidity files.

# Define a custom file type for Solidity if not already recognized
rg --type-add 'solidity:*.sol' --type solidity -A 3 'function\s+deposit\s*\(' --glob '!contracts/src/staking/IDepositContract.sol'

# Search for invocations of the `deposit` function in Solidity files
rg --type-add 'solidity:*.sol' --type solidity -A 1 '\.deposit\s*\('

Length of output: 2188

)
external
payable;
Expand Down
96 changes: 82 additions & 14 deletions contracts/test/staking/DepositContract.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ contract DepositContractTest is SoladyTest {

bytes32 internal constant STAKING_ASSET_SLOT = bytes32(0);

address internal operator = vm.addr(1);

/// @dev the deposit contract.
PermissionedDepositContract internal depositContract;

Expand All @@ -38,7 +40,8 @@ contract DepositContractTest is SoladyTest {
bytes("wrong_public_key"),
STAKING_CREDENTIALS,
32e9,
_create96Byte()
_create96Byte(),
operator
Comment on lines +43 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use pubKey parameter in deposit call for proper fuzz testing.

In testFuzz_DepositsWrongPubKey, the deposit function is called with a hardcoded bytes("wrong_public_key") instead of the pubKey parameter. To effectively fuzz test with various invalid public keys, use the pubKey parameter in the deposit call.

Apply this diff to fix the issue:

 depositContract.deposit(
-    bytes("wrong_public_key"),
+    pubKey,
     STAKING_CREDENTIALS,
     32e9,
     _create96Byte(),
     operator
 );

Committable suggestion was skipped due to low confidence.

);
}

Expand All @@ -49,7 +52,8 @@ contract DepositContractTest is SoladyTest {
bytes("wrong_public_key"),
STAKING_CREDENTIALS,
32e9,
_create96Byte()
_create96Byte(),
operator
);
}

Expand All @@ -61,15 +65,19 @@ contract DepositContractTest is SoladyTest {
vm.expectRevert(IDepositContract.InvalidCredentialsLength.selector);
vm.prank(depositor);
depositContract.deposit(
_create48Byte(), credentials, 32e9, _create96Byte()
_create48Byte(), credentials, 32e9, _create96Byte(), operator
);
}

function test_DepositWrongCredentials() public {
vm.expectRevert(IDepositContract.InvalidCredentialsLength.selector);
vm.prank(depositor);
depositContract.deposit(
VALIDATOR_PUBKEY, bytes("wrong_credentials"), 32e9, _create96Byte()
VALIDATOR_PUBKEY,
bytes("wrong_credentials"),
32e9,
_create96Byte(),
operator
Comment on lines +76 to +80
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Use consistent invalid credentials in test_DepositWrongCredentials.

In test_DepositWrongCredentials, consider using a consistent placeholder for invalid credentials similar to other tests. This enhances readability and consistency across tests.

Apply this diff for consistency:

             VALIDATOR_PUBKEY,
-            bytes("wrong_credentials"),
+            INVALID_CREDENTIALS,
             32e9,
             _create96Byte(),
             operator

Where you define INVALID_CREDENTIALS at the beginning of the contract:

+    bytes internal constant INVALID_CREDENTIALS = bytes("wrong_credentials");

Committable suggestion was skipped due to low confidence.

);
}

Expand All @@ -82,15 +90,32 @@ contract DepositContractTest is SoladyTest {
VALIDATOR_PUBKEY,
STAKING_CREDENTIALS,
uint64(amount),
_create96Byte()
_create96Byte(),
operator
);
}

function test_DepositWrongAmount() public {
vm.expectRevert(IDepositContract.InsufficientDeposit.selector);
vm.prank(depositor);
depositContract.deposit(
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 32e9 - 1, _create96Byte()
VALIDATOR_PUBKEY,
STAKING_CREDENTIALS,
32e9 - 1,
_create96Byte(),
operator
);
}

function test_DepositZeroOperator() public {
vm.expectRevert(IDepositContract.ZeroOperatorOnFirstDeposit.selector);
vm.prank(depositor);
depositContract.deposit(
VALIDATOR_PUBKEY,
STAKING_CREDENTIALS,
32e9,
_create96Byte(),
address(0)
);
}

Expand All @@ -101,8 +126,15 @@ contract DepositContractTest is SoladyTest {
emit IDepositContract.Deposit(
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 32e9, _create96Byte(), 0
);
emit IDepositContract.OperatorUpdated(
VALIDATOR_PUBKEY, operator, address(0)
);
depositContract.deposit{ value: 32 ether }(
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 32e9, _create96Byte()
VALIDATOR_PUBKEY,
STAKING_CREDENTIALS,
32e9,
_create96Byte(),
operator
);
}

Expand All @@ -115,7 +147,7 @@ contract DepositContractTest is SoladyTest {
vm.prank(depositor);
vm.expectRevert(IDepositContract.InsufficientDeposit.selector);
depositContract.deposit{ value: amountInGwei }(
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 0, _create96Byte()
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 0, _create96Byte(), operator
);
}

Expand All @@ -125,7 +157,7 @@ contract DepositContractTest is SoladyTest {
vm.prank(depositor);
vm.expectRevert(IDepositContract.InsufficientDeposit.selector);
depositContract.deposit{ value: amount }(
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 0, _create96Byte()
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 0, _create96Byte(), operator
);
}

Expand All @@ -137,7 +169,7 @@ contract DepositContractTest is SoladyTest {
vm.prank(depositor);
vm.expectRevert(IDepositContract.DepositNotMultipleOfGwei.selector);
depositContract.deposit{ value: amount }(
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 0, _create96Byte()
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 0, _create96Byte(), operator
);
}

Expand All @@ -147,15 +179,15 @@ contract DepositContractTest is SoladyTest {
vm.expectRevert(IDepositContract.DepositNotMultipleOfGwei.selector);
vm.prank(depositor);
depositContract.deposit{ value: amount }(
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 0, _create96Byte()
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 0, _create96Byte(), operator
);

amount = 32e9 - 1;
vm.deal(depositor, amount);
vm.expectRevert(IDepositContract.DepositNotMultipleOfGwei.selector);
vm.prank(depositor);
depositContract.deposit{ value: amount }(
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 0, _create96Byte()
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 0, _create96Byte(), operator
);
}

Expand All @@ -167,7 +199,7 @@ contract DepositContractTest is SoladyTest {
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 32 gwei, _create96Byte(), 0
);
depositContract.deposit{ value: 32 ether }(
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 0, _create96Byte()
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 0, _create96Byte(), operator
);
}

Expand All @@ -186,13 +218,49 @@ contract DepositContractTest is SoladyTest {
depositCount
);
depositContract.deposit{ value: 32 ether }(
VALIDATOR_PUBKEY, STAKING_CREDENTIALS, 0, _create96Byte()
VALIDATOR_PUBKEY,
STAKING_CREDENTIALS,
0,
_create96Byte(),
operator
);
++depositCount;
}
assertEq(depositContract.depositCount(), depositCount);
}

function test_UpdateOperatorNotCurrentOperator() public {
// Reverts if pubkey is not registered.
address newOperator = vm.addr(2);
vm.expectRevert(IDepositContract.NotCurrentOperator.selector);
vm.prank(operator);
depositContract.updateOperator(VALIDATOR_PUBKEY, newOperator);

// Reverts if pubkey is registered but the caller is not the current operator.
test_Deposit();
vm.expectRevert(IDepositContract.NotCurrentOperator.selector);
vm.prank(newOperator);
depositContract.updateOperator(VALIDATOR_PUBKEY, newOperator);
}

function test_UpdateOperatorZeroAddress() public {
test_Deposit();
vm.expectRevert(IDepositContract.ZeroAddress.selector);
vm.prank(operator);
depositContract.updateOperator(VALIDATOR_PUBKEY, address(0));
}

function test_UpdateOperator() public {
test_Deposit();
address newOperator = vm.addr(2);
vm.prank(operator);
vm.expectEmit(true, true, true, true);
emit IDepositContract.OperatorUpdated(
VALIDATOR_PUBKEY, newOperator, operator
);
depositContract.updateOperator(VALIDATOR_PUBKEY, newOperator);
}

function _credential(address addr) internal pure returns (bytes memory) {
return abi.encodePacked(bytes1(0x01), bytes11(0x0), addr);
}
Expand Down
7 changes: 5 additions & 2 deletions contracts/test/staking/PermissionedDepositContract.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ contract PermissionedDepositContract is DepositContract, Ownable {
bytes calldata pubkey,
bytes calldata withdrawal_credentials,
uint64 amount,
bytes calldata signature
bytes calldata signature,
address operator
Comment on lines +43 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Consider enhancing error handling.

The changes to the deposit function are correct and align with the modifications in the parent DepositContract. The new operator parameter is properly included in the function signature and passed to the superclass's deposit function.

Consider adding a check for the operator address to ensure it's not the zero address. This can prevent potential issues with invalid operators. You could add this check right after the existing authorization check:

if (operator == address(0)) revert InvalidOperator();

Don't forget to define the InvalidOperator error at the top of the contract if you decide to implement this suggestion.

Also applies to: 53-55

)
public
payable
Expand All @@ -49,7 +50,9 @@ contract PermissionedDepositContract is DepositContract, Ownable {
if (depositAuth[msg.sender] == 0) revert UnauthorizedDeposit();
--depositAuth[msg.sender];

super.deposit(pubkey, withdrawal_credentials, amount, signature);
super.deposit(
pubkey, withdrawal_credentials, amount, signature, operator
);
}

function allowDeposit(
Expand Down
Loading
Loading