fix:add missing fields #64

trestinlsd · 2024-05-22T03:55:07Z

Description

avs module add missing fields

Fix vulnerability GO-2024-2694 in github.com/cosmos/ibc-go/v7

Upgraded github.com/cosmos/ibc-go/v7 to version v7.4.0 to address GO-2024-2694 vulnerability.
Fixed potential reentrancy issue in ibc-hooks.
Updated dependencies to ensure code integrity and security.

cloud8little · 2024-05-23T11:02:48Z

Test passed with 745b278
avs owner address is added.

exocored q avs AVSInfo exo1l7m80efn6lfkdcng47rwy2rma6xsqywu6cwe2v --home ~/.tmp-exocored
info:
  asset_id:
  - "123"
  avs_address: exo1l7m80efn6lfkdcng47rwy2rma6xsqywu6cwe2v
  avs_owner_address: exo1nkh04qzxed98megzz53xx9a4uju599zg3rw0mj
  name: avs01
  operator_address:
  - exo18cggcpvwspnd5c6ny8wrqxpffj5zmhklprtnph

TimmyExogenous

It seems that the register and deregister functions of the AVS can still be called by any address, right?
If the owner address of the AVS is a parameter passed in during registration, then deregistration or updates would require signature verification of the owner address. Implementing such permission verification is relatively complex. Moreover, this PR only adds an owner address parameter initialization and does not address the permission control issues related to AVS operations. The key should be to use the "from" address in the msg call as the owner address. You can refer to the implementation of other similar functionalities for details.

cloud8little · 2024-05-27T07:52:23Z

Move to further milestone as the owner address needs to be well-designed.

mikebraver · 2024-05-27T15:09:55Z

precompiles/avs/avs.go

 		bz, err = p.AVSInfoRegisterOrDeregister(ctx, evm.Origin, contract, stateDB, method, args)
+	case MethodRegisterOperatorToAVS:


So won't you support operator deregister from AVS?

mikebraver · 2024-05-27T15:10:47Z

precompiles/avs/tx.go

 )

 const (
 	// AVSRegister defines the ABI method name for the avs
 	// related transaction.
-	MethodAVSAction = "AVSAction"
+	MethodAVSAction             = "AVSAction"
+	MethodRegisterOperatorToAVS = "registerOperatorToAVS"


mikebraver · 2024-05-27T15:14:46Z

precompiles/avs/tx.go

@@ -33,3 +45,35 @@ func (p Precompile) AVSInfoRegisterOrDeregister(
 	}
 	return method.Outputs.Pack(true)
 }
+
+func (p Precompile) OperatorInfoRegister(


The name of this function is confused, the operator is registered via the operator module, what you'd do is to register operator to avs, shouldn't this added in operator module?

Also need to support operator deregister from avs

mikebraver · 2024-05-27T15:18:48Z

x/avs/keeper/operator.go

+	return nil
+}
+
+func (k *Keeper) GetOperator(ctx sdk.Context, addr string) (operator types.OperatorInfo, err error) {


Why not get operator in operator module?

mikebraver

Many places need to be updated!

mikebraver · 2024-05-27T16:04:39Z

proto/exocore/avs/tx.proto

-  // registered operator of avs
-  repeated string operator_address = 3;
+  //  slash address of avs
+  string slash_addr = 3;
  // the owner who has permission for avs
  string avs_owner_address = 4;


Won't there's any case that avs has multiple owners or operators?

mikebraver · 2024-05-27T16:07:16Z

precompiles/avs/types.go

 	}
-	avsParams.AssetID = assetID
+	avsParams.UnbondingEpochs = unbondingEpochs


You defined both unbondingEpochs and Epochs in protobuf struct, why there's only unbondingEpochs set here?

mikebraver · 2024-05-27T16:07:58Z

proto/exocore/avs/tx.proto

+  EpochInfo avs_epoch = 8;
+}
+//  The information of OperatorInfo in opt-in avs
+message OperatorInfo {


Shouldn't it store the avs meta info within this struct?

mikebraver · 2024-05-27T16:10:26Z

x/avs/keeper/avs.go

+	sdk "github.com/cosmos/cosmos-sdk/types"
+)
+
+func (k *Keeper) GetAVSSupportedAssets(ctx sdk.Context, avsAddr string) ([]string, error) {


Please reuse the function GetAVSInfo to get avs info, and return res.AssetId then

mikebraver · 2024-05-27T16:10:36Z

x/avs/keeper/avs.go

+
+	return ret.AssetId, nil
+}
+func (k *Keeper) GetAVSSlashContract(ctx sdk.Context, avsAddr string) (string, error) {


mikebraver · 2024-05-27T16:12:38Z

Please also fix all action errors

precompiles/avs/tx.go

mikebraver · 2024-05-29T08:26:27Z

x/avs/types/types.go

@@ -1 +1,19 @@
 package types
+
+func ContainsString(slice []string, target string) bool {


No need to re-implement it, just use import slices and slices.Contains()

precompiles/avs/tx.go

mikebraver

Can merge it after all comments and action error are resolved

bwhour · 2024-05-29T10:01:37Z

precompiles/avs/abi.json

-				"name": "assetID",
-				"type": "string"
+				"internalType": "uint64",
+				"name": "unbondingEpochs",


unbondingEpoch: it's better named it unbondingTime or unbondingPeriod

bwhour · 2024-05-29T10:14:06Z

precompiles/avs/avs.sol

        uint64 action,
-        string memory avsOwnerAddress,
-        string memory assetID
+        uint64 minimumDelegation,


is this minimumSelfDelegation ?

Minimum stake quantity for Operator

bwhour · 2024-05-29T10:14:48Z

precompiles/avs/avs.sol

-        string memory avsOwnerAddress,
-        string memory assetID
+        uint64 minimumDelegation,
+        uint64 unbondingEpochs


unbondingTime/unbondingPeriod would be better.

bwhour · 2024-05-29T10:17:42Z

precompiles/avs/tx.go

+
+	exocmn "github.com/ExocoreNetwork/exocore/precompiles/common"
+	util "github.com/ExocoreNetwork/exocore/utils"
+	avstypes "github.com/ExocoreNetwork/exocore/x/avs/keeper"


this should be avskeeper not avstypes. please change the related places.

bwhour · 2024-05-29T10:18:30Z

precompiles/avs/tx.go

+	if err != nil {
+		return nil, errorsmod.Wrap(err, "parse args error")
+	}
+	avsAddress, err := util.ProcessAvsAddress(contract.Address().String())


ProcessAVSAddress

bwhour · 2024-05-29T10:25:39Z

precompiles/avs/tx.go

-	// related transaction.
-	MethodAVSAction = "AVSAction"
+	MethodAVSAction      = "AVSAction"
+	MethodOperatorAction = "OperatorOptAction"
 )

 // AVSInfoRegister register the avs related information and change the state in avs keeper module.
 func (p Precompile) AVSInfoRegisterOrDeregister(


AVSInfoRegisterOrDeregister:
Although this function name describes its functionality, it is a bit long and not specific enough. Consider using a more concise and clear name.It is recommended to change it to RegisterOrDeregisterAVSInfo, which better follows the action + object naming convention for functions.

OperatorBindingAvs:
This function name can also be more specific, clearly indicating that it binds an operator to AVS.
It is recommended to change it to BindOperatorToAVS, which more clearly expresses the function's purpose.

bwhour · 2024-05-29T10:31:36Z

x/avs/keeper/avs.go

+func (k *Keeper) GetAVSSlashContract(ctx sdk.Context, avsAddr string) (string, error) {
+	avsInfo, err := k.GetAVSInfo(ctx, avsAddr)
+	if err != nil {
+		return "", errorsmod.Wrap(err, fmt.Sprintf("GetAVSSupportedAssets: key is %s", avsAddr))


this function log GetAVSSupportedAssets is not right

bwhour · 2024-05-29T10:32:03Z

x/avs/keeper/avs.go

+func (k *Keeper) GetAVSMinimumSelfDelegation(ctx sdk.Context, avsAddr string) (sdkmath.LegacyDec, error) {
+	avsInfo, err := k.GetAVSInfo(ctx, avsAddr)
+	if err != nil {
+		return sdkmath.LegacyNewDec(0), errorsmod.Wrap(err, fmt.Sprintf("GetAVSSupportedAssets: key is %s", avsAddr))


here function log : GetAVSSupportedAssets

bwhour · 2024-05-29T11:20:48Z

x/avs/keeper/keeper.go

+		SlashAddr:          params.SlashContractAddr,
+		AvsOwnerAddress:    params.AvsOwnerAddress,
+		AssetId:            params.AssetID,
+		AvsUnbondingEpochs: uint32(params.MinimumDelegation),


this param has a lot of types in different places: unit32, uint64, int64. please have a check.
this assignment is strange :

AvsUnbondingEpochs: uint32(params.MinimumDelegation),
MinimumDelegation: sdk.NewIntFromUint64(params.UnbondingEpochs),

bwhour · 2024-05-29T11:25:09Z

x/avs/keeper/keeper.go

+	action := params.Action
+
+	if action == RegisterAction && avsInfo != nil {
+		return errorsmod.Wrap(types.ErrAlreadyRegistered, fmt.Sprintf("the error input arg is:%s", params.AvsAddress))


can replace this with specific error " the avsaddress is :%s",

bwhour · 2024-05-29T11:26:38Z

x/avs/keeper/keeper.go

+	}
+
+	if avsInfo == nil {
+		return errorsmod.Wrap(types.ErrUnregisterNonExistent, fmt.Sprintf("the error input arg is:%s", avsInfo))


avsInfo == nil so this line has no meaning print: fmt.Sprintf("the error input arg is:%s", avsInfo)

unregistration must be done with data available

bwhour · 2024-05-29T11:29:00Z

x/avs/keeper/keeper.go

 	}
 	avsInfo, err := k.GetAVSInfo(ctx, params.AvsAddress)
-	if err != nil {
+	if err != nil || avsInfo.GetInfo() == nil {


there are two lines call avsInfo.GetInfo(), can we call once ?

bwhour · 2024-05-29T11:29:18Z

x/avs/keeper/keeper.go

 		return errorsmod.Wrap(err, fmt.Sprintf("error occurred when get avs info %s", avsInfo))
 	}
-	if avsInfo.Info.AvsOwnerAddress != params.AvsOwnerAddress {
-		return errorsmod.Wrap(err, fmt.Sprintf("not qualified to deregister %s", avsInfo))
+	avs := avsInfo.GetInfo()


see above comment

bwhour · 2024-05-29T12:13:36Z

x/avs/keeper/keeper.go

-	ret.OperatorAddress = append(ret.OperatorAddress, operatorAddress)
-	ret.AssetId = append(ret.AssetId, assetID)
+
+	AVSInfo := avs


this AVSInfo variable is the same as types.AVSInfo

bwhour

some comments left need to be fixed.

trestinlsd requested review from mikebraver, cloud8little and TimmyExogenous May 22, 2024 03:56

cloud8little approved these changes May 23, 2024

View reviewed changes

cloud8little added this to the Testnet V3 milestone May 24, 2024

mikebraver approved these changes May 24, 2024

View reviewed changes

TimmyExogenous reviewed May 24, 2024

View reviewed changes

github-actions bot added the C:Proto label May 27, 2024

cloud8little removed this from the Testnet V3 milestone May 27, 2024

trestinlsd force-pushed the develop branch from 1f07b2d to 7212bf1 Compare May 27, 2024 10:16

mikebraver reviewed May 27, 2024

View reviewed changes

mikebraver requested changes May 27, 2024

View reviewed changes

mikebraver reviewed May 27, 2024

View reviewed changes

github-actions bot added the C:CLI label May 28, 2024

github-advanced-security bot found potential problems May 28, 2024

View reviewed changes

precompiles/avs/tx.go Fixed Show fixed Hide fixed

mikebraver reviewed May 29, 2024

View reviewed changes

refactor: refactor avs and add test

3225f1f

trestinlsd force-pushed the develop branch from f8cdc00 to 3225f1f Compare May 29, 2024 08:30

github-advanced-security bot found potential problems May 29, 2024

View reviewed changes

precompiles/avs/tx.go Fixed Show fixed Hide fixed

mikebraver approved these changes May 29, 2024

View reviewed changes

fix:fix golangci-lint error

3366e38

bwhour reviewed May 29, 2024

View reviewed changes

bwhour requested changes May 29, 2024

View reviewed changes

trestinlsd force-pushed the develop branch from 63c0fe0 to c08369a Compare May 30, 2024 03:10

trestinlsd requested a review from bwhour May 30, 2024 03:13

fix:fix issues in comments

1eee8ee

trestinlsd force-pushed the develop branch from c08369a to 1eee8ee Compare May 30, 2024 03:28

Merge branch 'ExocoreNetwork:develop' into develop

ab52f86

bwhour approved these changes May 30, 2024

View reviewed changes

trestinlsd merged commit 34cf62f into ExocoreNetwork:develop May 30, 2024
19 checks passed

cloud8little mentioned this pull request May 30, 2024

AVS owner Address Missing in the AVSInfo #62

Closed

		bz, err = p.AVSInfoRegisterOrDeregister(ctx, evm.Origin, contract, stateDB, method, args)
		case MethodRegisterOperatorToAVS:

		@@ -1 +1,19 @@
		package types

		func ContainsString(slice []string, target string) bool {

fix:add missing fields #64

fix:add missing fields #64

Conversation

trestinlsd commented May 22, 2024 • edited Loading

Description

cloud8little commented May 23, 2024

TimmyExogenous left a comment

Choose a reason for hiding this comment

cloud8little commented May 27, 2024

Choose a reason for hiding this comment

trestinlsd May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebraver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebraver commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebraver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwhour May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwhour left a comment

Choose a reason for hiding this comment

trestinlsd commented May 22, 2024 •

edited

Loading

trestinlsd May 29, 2024 •

edited

Loading

bwhour May 29, 2024 •

edited

Loading