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

M-1 Discrepancy between operator management in smart contract and L1 #302

Open
Lambda6080604052 opened this issue Oct 24, 2024 · 0 comments · May be fixed by #358
Open

M-1 Discrepancy between operator management in smart contract and L1 #302

Lambda6080604052 opened this issue Oct 24, 2024 · 0 comments · May be fixed by #358
Assignees
Labels
bug Something isn't working

Comments

@Lambda6080604052
Copy link

Description and context

The smart contracts contain a function removeOperator that removes a specified operator (

emit RemoveOperator(uncmpPubkey, operator);
):

    /// @notice Removes an operator for a delegator.
    /// @param uncmpPubkey 65 bytes uncompressed secp256k1 public key.
    /// @param operator The operator address to remove.
    function removeOperator(
        bytes calldata uncmpPubkey,
        address operator
    ) external verifyUncmpPubkeyWithExpectedAddress(uncmpPubkey, msg.sender) {
        emit RemoveOperator(uncmpPubkey, operator);
    }

However, the operator argument of the emitted event is completely ignored when this event is processed (

if err := k.DelegatorOperatorAddress.Remove(ctx, depositorAddr.String()); err != nil {
):

func (k Keeper) ProcessRemoveOperator(ctx context.Context, ev *bindings.IPTokenStakingRemoveOperator) error {
    delCmpPubkey, err := UncmpPubKeyToCmpPubKey(ev.UncmpPubkey)
    if err != nil {
        return errors.Wrap(err, "compress depositor pubkey")
    }
    depositorPubkey, err := k1util.PubKeyBytesToCosmos(delCmpPubkey)
    if err != nil {
        return errors.Wrap(err, "depositor pubkey to cosmos")
    }

    depositorAddr := sdk.AccAddress(depositorPubkey.Address().Bytes())

    if err := k.DelegatorOperatorAddress.Remove(ctx, depositorAddr.String()); err != nil {
        return errors.Wrap(err, "delegator operator address map remove")
    }

    return nil
}

The code will just remove the current operator.

A similar discrepancy also exists for addOperator. The smart contract mentions that an operator is added (which makes sense based on the function name).

    /// @notice Adds an operator for a delegator.
    /// @param uncmpPubkey 65 bytes uncompressed secp256k1 public key.
    /// @param operator The operator address to add.
    function addOperator(
        bytes calldata uncmpPubkey,
        address operator
    ) external payable verifyUncmpPubkeyWithExpectedAddress(uncmpPubkey, msg.sender) chargesFee {
        emit AddOperator(uncmpPubkey, operator);
    }

However, in the L1 code, there can only be ever one operator per delegator (stored in the k.DelegatorOperatorAddress map) and the current operator is overwritten whenever a new one is added.

Solution recommendation

If multiple operators should be supported, we recommend changing the L1 code and data structures to store all operators. If not, we recommend changing the smart contracts (function name + documentation for addOperator, removal of unnecessary event for removeOperator).

@Lambda6080604052 Lambda6080604052 added the bug Something isn't working label Oct 24, 2024
@Ramarti Ramarti self-assigned this Oct 31, 2024
@Ramarti Ramarti linked a pull request Nov 14, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants