Prevent ACL admins to remove themselves (#1578)

* As an admin I can not change my own role
0xPolygon · Jun 2, 2023 · d230546 · d230546
1 parent 994e190
commit d230546
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 16 deletions.
diff --git a/e2e-polybft/e2e/acls_test.go b/e2e-polybft/e2e/acls_test.go
@@ -92,7 +92,7 @@ func TestE2E_AllowList_ContractDeployment(t *testing.T) {
 
 	{
 		// Step 4. 'adminAddr' sends a transaction to enable 'targetAddr'.
-		input, _ := addresslist.SetEnabledSignatureFunc.Encode([]interface{}{targetAddr})
+		input, _ := addresslist.SetEnabledFunc.Encode([]interface{}{targetAddr})
 
 		adminSetTxn := cluster.MethodTxn(t, admin, contracts.AllowListContractsAddr, input)
 		require.NoError(t, adminSetTxn.Wait())
@@ -110,7 +110,7 @@ func TestE2E_AllowList_ContractDeployment(t *testing.T) {
 	{
 		// Step 6. 'targetAddr' cannot enable other accounts since it is not an admin
 		// (The transaction fails)
-		input, _ := addresslist.SetEnabledSignatureFunc.Encode([]interface{}{types.ZeroAddress})
+		input, _ := addresslist.SetEnabledFunc.Encode([]interface{}{types.ZeroAddress})
 
 		adminSetFailTxn := cluster.MethodTxn(t, target, contracts.AllowListContractsAddr, input)
 		require.NoError(t, adminSetFailTxn.Wait())
@@ -184,7 +184,7 @@ func TestE2E_BlockList_ContractDeployment(t *testing.T) {
 
 	{
 		// Step 4. 'adminAddr' sends a transaction to enable 'targetAddr'.
-		input, _ := addresslist.SetEnabledSignatureFunc.Encode([]interface{}{targetAddr})
+		input, _ := addresslist.SetEnabledFunc.Encode([]interface{}{targetAddr})
 
 		adminSetTxn := cluster.MethodTxn(t, admin, contracts.BlockListContractsAddr, input)
 		require.NoError(t, adminSetTxn.Wait())
@@ -202,7 +202,7 @@ func TestE2E_BlockList_ContractDeployment(t *testing.T) {
 	{
 		// Step 6. 'targetAddr' cannot enable other accounts since it is not an admin
 		// (The transaction fails)
-		input, _ := addresslist.SetEnabledSignatureFunc.Encode([]interface{}{types.ZeroAddress})
+		input, _ := addresslist.SetEnabledFunc.Encode([]interface{}{types.ZeroAddress})
 
 		adminSetFailTxn := cluster.MethodTxn(t, target, contracts.BlockListContractsAddr, input)
 		require.NoError(t, adminSetFailTxn.Wait())
@@ -268,7 +268,7 @@ func TestE2E_AllowList_Transactions(t *testing.T) {
 
 	{
 		// Step 3. 'adminAddr' sends a transaction to enable 'targetAddr'.
-		input, _ := addresslist.SetEnabledSignatureFunc.Encode([]interface{}{targetAddr})
+		input, _ := addresslist.SetEnabledFunc.Encode([]interface{}{targetAddr})
 
 		adminSetTxn := cluster.MethodTxn(t, admin, contracts.AllowListTransactionsAddr, input)
 		require.NoError(t, adminSetTxn.Wait())
@@ -285,13 +285,23 @@ func TestE2E_AllowList_Transactions(t *testing.T) {
 	{
 		// Step 5. 'targetAddr' cannot enable other accounts since it is not an admin
 		// (The transaction fails)
-		input, _ := addresslist.SetEnabledSignatureFunc.Encode([]interface{}{types.ZeroAddress})
+		input, _ := addresslist.SetEnabledFunc.Encode([]interface{}{types.ZeroAddress})
 
 		adminSetFailTxn := cluster.MethodTxn(t, target, contracts.AllowListTransactionsAddr, input)
 		require.NoError(t, adminSetFailTxn.Wait())
 		require.True(t, adminSetFailTxn.Failed())
 		expectRole(t, cluster, contracts.AllowListTransactionsAddr, types.ZeroAddress, addresslist.NoRole)
 	}
+
+	{
+		// Step 6. 'adminAddr' sends a transaction to disable himself.
+		input, _ := addresslist.SetNoneFunc.Encode([]interface{}{adminAddr})
+
+		noneSetTxn := cluster.MethodTxn(t, admin, contracts.AllowListTransactionsAddr, input)
+		require.NoError(t, noneSetTxn.Wait())
+		require.True(t, noneSetTxn.Failed())
+		expectRole(t, cluster, contracts.AllowListTransactionsAddr, adminAddr, addresslist.AdminRole)
+	}
 }
 
 func TestE2E_BlockList_Transactions(t *testing.T) {
@@ -339,7 +349,7 @@ func TestE2E_BlockList_Transactions(t *testing.T) {
 	{
 		// Step 3. 'targetAddr' cannot enable other accounts since it is not an admin
 		// (The transaction fails)
-		input, _ := addresslist.SetEnabledSignatureFunc.Encode([]interface{}{types.ZeroAddress})
+		input, _ := addresslist.SetEnabledFunc.Encode([]interface{}{types.ZeroAddress})
 
 		adminSetFailTxn := cluster.MethodTxn(t, target, contracts.BlockListTransactionsAddr, input)
 		require.NoError(t, adminSetFailTxn.Wait())
@@ -349,7 +359,7 @@ func TestE2E_BlockList_Transactions(t *testing.T) {
 
 	{
 		// Step 4. 'adminAddr' sends a transaction to enable 'targetAddr'.
-		input, _ := addresslist.SetEnabledSignatureFunc.Encode([]interface{}{targetAddr})
+		input, _ := addresslist.SetEnabledFunc.Encode([]interface{}{targetAddr})
 
 		adminSetTxn := cluster.MethodTxn(t, admin, contracts.BlockListTransactionsAddr, input)
 		require.NoError(t, adminSetTxn.Wait())

diff --git a/e2e-polybft/e2e/bridge_test.go b/e2e-polybft/e2e/bridge_test.go
@@ -922,7 +922,7 @@ func TestE2E_Bridge_Transfers_AccessLists(t *testing.T) {
 		require.Error(t, err)
 
 		{
-			input, _ := addresslist.SetEnabledSignatureFunc.Encode([]interface{}{senderAccount.Ecdsa.Address()})
+			input, _ := addresslist.SetEnabledFunc.Encode([]interface{}{senderAccount.Ecdsa.Address()})
 			enableSetTxn := cluster.MethodTxn(t, admin, contracts.AllowListBridgeAddr, input)
 			require.NoError(t, enableSetTxn.Wait())
 			expectRole(t, cluster, contracts.AllowListBridgeAddr, types.Address(senderAccount.Ecdsa.Address()), addresslist.EnabledRole)
@@ -940,7 +940,7 @@ func TestE2E_Bridge_Transfers_AccessLists(t *testing.T) {
 		require.NoError(t, err)
 
 		{
-			input, _ := addresslist.SetEnabledSignatureFunc.Encode([]interface{}{senderAccount.Ecdsa.Address()})
+			input, _ := addresslist.SetEnabledFunc.Encode([]interface{}{senderAccount.Ecdsa.Address()})
 			disableSetTxn := cluster.MethodTxn(t, admin, contracts.BlockListBridgeAddr, input)
 			require.NoError(t, disableSetTxn.Wait())
 			expectRole(t, cluster, contracts.BlockListBridgeAddr, types.Address(senderAccount.Ecdsa.Address()), addresslist.EnabledRole)

diff --git a/state/runtime/addresslist/addresslist.go b/state/runtime/addresslist/addresslist.go
@@ -12,10 +12,10 @@ import (
 
 // list of function methods for the address list functionality
 var (
-	SetAdminFunc            = abi.MustNewMethod("function setAdmin(address)")
-	SetEnabledSignatureFunc = abi.MustNewMethod("function setEnabled(address)")
-	SetNoneFunc             = abi.MustNewMethod("function setNone(address)")
-	ReadAddressListFunc     = abi.MustNewMethod("function readAddressList(address) returns (uint256)")
+	SetAdminFunc        = abi.MustNewMethod("function setAdmin(address)")
+	SetEnabledFunc      = abi.MustNewMethod("function setEnabled(address)")
+	SetNoneFunc         = abi.MustNewMethod("function setNone(address)")
+	ReadAddressListFunc = abi.MustNewMethod("function readAddressList(address) returns (uint256)")
 )
 
 // list of gas costs for the operations
@@ -55,6 +55,7 @@ var (
 	errInputTooShort       = fmt.Errorf("wrong input size, expected 32")
 	errFunctionNotFound    = fmt.Errorf("function not found")
 	errWriteProtection     = fmt.Errorf("write protection")
+	errAdminSelfRemove     = fmt.Errorf("cannot remove admin role from caller")
 )
 
 func (a *AddressList) runInputCall(caller types.Address, input []byte,
@@ -102,7 +103,7 @@ func (a *AddressList) runInputCall(caller types.Address, input []byte,
 	var updateRole Role
 	if bytes.Equal(sig, SetAdminFunc.ID()) {
 		updateRole = AdminRole
-	} else if bytes.Equal(sig, SetEnabledSignatureFunc.ID()) {
+	} else if bytes.Equal(sig, SetEnabledFunc.ID()) {
 		updateRole = EnabledRole
 	} else if bytes.Equal(sig, SetNoneFunc.ID()) {
 		updateRole = NoRole
@@ -125,6 +126,11 @@ func (a *AddressList) runInputCall(caller types.Address, input []byte,
 		return nil, gasUsed, runtime.ErrNotAuth
 	}
 
+	// An admin can not remove himself from the list
+	if addrRole == AdminRole && caller == inputAddr {
+		return nil, gasUsed, errAdminSelfRemove
+	}
+
 	a.SetRole(inputAddr, updateRole)
 
 	return nil, gasUsed, nil

diff --git a/state/runtime/addresslist/addresslist_test.go b/state/runtime/addresslist/addresslist_test.go
@@ -138,7 +138,7 @@ func TestAddressList_WriteOp_Full(t *testing.T) {
 		role   Role
 	}{
 		{SetAdminFunc, AdminRole},
-		{SetEnabledSignatureFunc, EnabledRole},
+		{SetEnabledFunc, EnabledRole},
 		{SetNoneFunc, NoRole},
 	}