registry:libs.rs - `remove_agent` returns error when the agent has `weight > 0`

[hats-bug-reporter[bot]]

Github username: @aktech297 Twitter username: kaka Submission hash (on-chain): 0x6d13c3fac0fb5a0d86d30c116de104dfa26bdbac0480df7971ed3338dd32b317 Severity: medium

Description: Description\ An agent can not be removed using funcation call to remove_agent, as the function return error when the agent has weight >0

1. Proof of Concept (PoC) File

The adming who has the RemoveAgent role can call the function remove_agent and remove.

Following code block can be seen here.

lib.rs#L220-L238

            if let Some(index) = self.agents.iter().position(|a| a.address == account) {
                let weight = self.agents[index].weight;

                // Do not delete agents with active weight (and possible bonded AZERO)
                if weight > 0 { ----->>> @@ audit - checks > 0 and return error
                    return Err(RegistryError::ActiveAgent);
                }

                self.total_weight -= weight; --->>> deduct the weight from total weight
                self.agents.remove(index);

                Self::env().emit_event(
                    AgentDeleted {
                        agent: account,
                    }
                );
            } else {
                return Err(RegistryError::AgentNotFound);
            }

As per the implementation, a direct function call to remove_agent should be able to remove agent.

But if the agent has wieght > 0, the above call would return error.

2. Revised Code File (Optional)

I think below weight check could be removed. Since the function call is made by the admin.

lib.rs#L223-L226

                // Do not delete agents with active weight (and possible bonded AZERO)
                if weight > 0 {
                    return Err(RegistryError::ActiveAgent);
                }

[0xmahdirostami]

Thanks for the submission, the check is intended, if the owner wants to remove an agent, it must first set the weight to zero and then remove it.

// Do not delete agents with active weight (and possible bonded AZERO)

[aktech297]

Hi ser, Please check, the logic returns error if weight > 0.

Also, if you see the remove agent logic,

https://github.com/hats-finance/Kintsu-0x7d70f9442af3a9a0a734fa6a1b4857f25518e9d2/blob/c9bdc853b18c305de832307b91a9bca0f281f71e/src/registry/lib.rs#L228-L229

                self.total_weight -= weight;
                self.agents.remove(index);

it deduct the weight properly and remove the agent from the vector. This is sufficient to remove the agent. no need to set to zero which is extra work we think.

[0xmahdirostami]

self.total_weight -= weight; this is extra and redundant should be removed,(i think it's from previous implementation)

the process of removing an agent is as follows:

1. update_agents and set agent to zero
2. remove agent the function doesn't allow removing agents with + weight. (it is by design)

[aktech297]

update agent has the below logic,

lib.rs#L180-L186

                    self.total_weight -= old_weight;
                    self.total_weight += new_weight;

                    self.agents[index] = Agent {
                        address: account,
                        weight: new_weight,
                    };

it is adding new_weight. Nowhere it is setting the value to zero.

The purpose of update agent and remove agent is different.

[0xmahdirostami]

new_weight will be zero for our case.

[aktech297]

I am not sure whether this is intended. May be development team can decide on this design.

There are lot of assumptions I can see in the function logic. in fact the name are different and functionality is different.

Other point to see here is the update agent function and remove agent function are not atomic.

in your assumption, first the update agent is adding the agent with zero value and then emitting the event. after that the admin make a call to remove the agent. not sure how this could assumed to set the value are zero.

[0xmahdirostami]

sponsors will review as well.

[bgibers]

Thanks for the submission, the check is intended, if the owner wants to remove an agent, it must first set the weight to zero and then remove it.

// Do not delete agents with active weight (and possible bonded AZERO)

self.total_weight -= weight; this is extra and redundant should be removed,(i think it's from previous implementation)

the process of removing an agent is as follows:

1. update_agents and set agent to zero
2. remove agent the function doesn't allow removing agents with + weight. (it is by design)

Both of these statements are completely correct @0xmahdirostami. The redundant step is subtracting nothing.

An agent should have its weight set to 0 before removing, failure to do so will result in AZERO still being bonded

[aktech297]

Hi @bgibers thanks for your comments.

in fact, we stuck with the n number of implicit assumption here.

we are really surprised to see the logic here.

first admin calls the update agent and then and update weights (unnecessary code block)

event is emitted with all the data.

after this function call, again admin calls the remove agent and removes the agent..

we would suggest to make it in single call

in fact, you could have used the remove agent function itself.

This will save unnecessary function execution which could save some gas and makes the code flow more meaningful.

[bgibers]

Hi @bgibers thanks for your comments.

in fact, we stuck with the n number of implicit assumption here.

we are really surprised to see the logic here.

first admin calls the update agent and then and update weights (unnecessary code block)

event is emitted with all the data.

after this function call, again admin calls the remove agent and removes the agent..

we would suggest to make it in single call

in fact, you could have used the remove agent function itself.

This will save unnecessary function execution which could save some gas and makes the code flow more meaningful.

I agree that some clean up can/should be implemented 💯

[aktech297]

N number of implicit assumptions diluting the competitive spirit.. @bgibers