From 4f1c8d19f29113c5e27ad841803f8e2722ceb543 Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 16 Mar 2023 20:31:35 +0100 Subject: [PATCH 1/2] refactor(Acl): reorder state modifications --- near-plugins-derive/src/access_controllable.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/near-plugins-derive/src/access_controllable.rs b/near-plugins-derive/src/access_controllable.rs index cd6c788..87cfc4d 100644 --- a/near-plugins-derive/src/access_controllable.rs +++ b/near-plugins-derive/src/access_controllable.rs @@ -207,8 +207,17 @@ pub fn access_controllable(attrs: TokenStream, item: TokenStream) -> TokenStream return None; } + // The following state modifications can be considered atomic: They happen in the + // current `FunctionCall` action because no promises or cross contract calls are + // scheduled. + // + // If this ever changes, it should be avoided that revoking `current_super_admin` + // succeeds and adding `account_id` as super-admin fails. This could lock contracts + // in a state without super-admins. To protect against this scenario, the new + // super-admin is added first. + let is_new_super_admin = self.add_super_admin_unchecked(&account_id); self.revoke_super_admin_unchecked(¤t_super_admin); - Some(self.add_super_admin_unchecked(&account_id)) + Some(is_new_super_admin) } /// Revokes super-admin permissions from `account_id` without checking any From b2df328b92578c912ff5a1197919cdd0414353a3 Mon Sep 17 00:00:00 2001 From: Moritz Date: Fri, 17 Mar 2023 10:18:42 +0100 Subject: [PATCH 2/2] Return early on transfer to self --- near-plugins-derive/src/access_controllable.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/near-plugins-derive/src/access_controllable.rs b/near-plugins-derive/src/access_controllable.rs index 87cfc4d..e6d5ed5 100644 --- a/near-plugins-derive/src/access_controllable.rs +++ b/near-plugins-derive/src/access_controllable.rs @@ -215,7 +215,14 @@ pub fn access_controllable(attrs: TokenStream, item: TokenStream) -> TokenStream // succeeds and adding `account_id` as super-admin fails. This could lock contracts // in a state without super-admins. To protect against this scenario, the new // super-admin is added first. - let is_new_super_admin = self.add_super_admin_unchecked(&account_id); + if account_id == ¤t_super_admin { + // That means Alice called `acl_transfer_super_admin(Alice)`, which should be a + // no-op and return `Some(true)`. However, the operations below would first add + // and then revoke Alice as super-admin, meaning Alice wouldn't be super-admin + // anymore. We return early to avoid that. + return Some(true); + } + let is_new_super_admin = self.add_super_admin_unchecked(account_id); self.revoke_super_admin_unchecked(¤t_super_admin); Some(is_new_super_admin) }