cault : data.rs : get_weight_imbalances did not check whether the agent has valid stake amount or not.

[hats-bug-reporter[bot]]

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

Description: Description\ Without checking the staked_amount_current, the function get_weight_imbalances calculates the positve diff and negative diff. This might lead to unexpected allocation to the agents during bonding process.

Impact\ Though the agent has not staked anything who is added recently or might be removed shortly by the admin, the function call delegate_bonding would end up with fund allocation to the agents.

Attachments

1. Proof of Concept (PoC) File

The function get_weight_imbalances calculates the total_staked, pos_diff, neg_diff, stakes and imbalances

data.rs#L167-L183

for a in agents.into_iter() {
            let staked_amount_current = query_staked_value(a.address) as i128; @@ audit - staked_amount_current  is not validated.
            let staked_amount_optimal = if total_weight > 0 {
                ((a.weight as u128 * total_pooled) / total_weight as u128) as i128
            } else {
                0
            };
            let diff = staked_amount_current - staked_amount_optimal;
            if diff > 0 {
                pos_diff += diff as u128;
            } else if diff < 0 {
                neg_diff += -diff as u128;
            }
            total_staked += staked_amount_current as u128;
            stakes.push(staked_amount_current as u128);
            imbalances.push(diff);
        }

As shown above, the diff is calculated between staked_amount_current and staked_amount_optimal. But there is no check to validate for staked_amount_current > 0.

This mean, even if the agent is not staking anything, the above logic says negative imbalance which is purely weighted value.

The function delegate_bonding uses this imbalance value to calculate the and distribute,

data.rs#L216-L222

            // Distribute to under-allocated agents
            // Weighted by agent imbalance
            let phase1_amount = if imbalances[i] < 0 {
                phase1 * (-imbalances[i] as u128) / neg_diff
            } else {
                0
            };

and the deposit for the agent.

data.rs#L257-L266

        // Deposit
        for (i, a) in agents.iter().enumerate() {
            let deposit_amount = deposit_amounts[i];
            if deposit_amount > 0 {
                debug_println!("Depositing {} into agent #{}", deposit_amount, i);
                if let Err(e) = call_deposit(a.address, deposit_amount) {
                    return Err(VaultError::InternalError(e));
                }
            }
        }

2. Revised Code File (Optional)

We request to check for valid staked amount when calculating the imbalance value in the get_weight_imbalances function.

data.rs#L167-L179

        for a in agents.into_iter() {
            let staked_amount_current = query_staked_value(a.address) as i128;
            let staked_amount_optimal = if total_weight > 0 {
                ((a.weight as u128 * total_pooled) / total_weight as u128) as i128
            } else {
                0
            };
            if staked_amount_optimal <= 0 {  ---- audit fix start

            let diff = staked_amount_current - staked_amount_optimal;
            if diff > 0 {
                pos_diff += diff as u128;
            } else if diff < 0 {
                neg_diff += -diff as u128;
            }
    } --- audit fix end

[aktech297]

Sorry , the fix should be if staked_amount_optimal > 0 {

    for a in agents.into_iter() {
        let staked_amount_current = query_staked_value(a.address) as i128;
        let staked_amount_optimal = if total_weight > 0 {
            ((a.weight as u128 * total_pooled) / total_weight as u128) as i128
        } else {
            0
        };
        if staked_amount_optimal > 0 {  ---- audit fix start

        let diff = staked_amount_current - staked_amount_optimal;
        if diff > 0 {
            pos_diff += diff as u128;
        } else if diff < 0 {
            neg_diff += -diff as u128;
        }
} --- audit fix end

[0xmahdirostami]

Thanks for submission.

Without checking the staked_amount_current, the function get_weight_imbalances calculates the positve diff and negative diff. This might lead to unexpected allocation to the agents during bonding process.

As shown above, the diff is calculated between staked_amount_current and staked_amount_optimal. But there is no check to validate for staked_amount_current > 0.

i couldn't get you, you are pointing the check for staked_amount_current, however in your revised code, you are adding a check for if staked_amount_optimal > 0 { ---- audit fix start.

if you are pointing a check for staked_amount_current, I should say that there are scenarios in which agents don't have balance(staked_amount_current=0) but should have balance so we figure out this and add funds to it.

if you are pointing a check for staked_amount_optimal, I should say that there are scenarios in which agents shouldn't have a balance(staked_amount_optimal=0) but have a balance so we figure out this and withdraw funds from it.

both staked_amount_optimal and staked_amount_current could be zero.

[aktech297]

Hi.. my fix should point to the swap staked_amount_current. I made mistake in the correction again. But the case where the agent will not staked anything could be during the starting stage i.e initially agent is added with weightage. Or if they are going to be removed ..

Both cases could cause issue in this. Lets say, if the agent is going to removed, just before removing , calling this function could allocate the funds for agents.

[0xmahdirostami]

If an agent is going to remove, the owner, add 0 weight to that agent. So staked_amount_optimal will be zero.

There are two scenarios here:

1. if an agent has staked funds: the diff will be positive, so in the delegate_bonding function, the pool will not be considered.
2. if an agent doesn't have staked funds: the diff will be 0, so again in the delegate_bonding function, the pool will not be considered.

If I miss anything, let me know.

[aktech297]

Hi ser.

I am aware that when agent is removed , the weight will be set to 0.

But what if this function called just before removing the agent .this is the case I am thinking

[0xmahdirostami]

Got you, the code just looks at the weight of the agent, how does the code figure out whether the agent is removing or not?

when agent is removed , the weight will be set to 0.

The owner couldn't remove the agent with + weight, first set it to 0 then remove it (whenever he wants).

[aktech297]

I need to add few more points.. pls give me some time.. I will be back, as I am outside now

[0xmahdirostami]

It would be better to create a POC(in code) or describe a scenario (step by step).

[aktech297]

Hi Ser,

The agent can have zero staked_amount_current in the following cases.

1. When agent is added.
2. When agent un-bond fully from the pool.
3. When agent is slashed fully.

The current function call flow to stake through agent is,

user calls stake with AZERO. and stake calls the delegate_bonding

Inside the delegate_bonding function, the agent is allocated the AZERO based on the weightage value.

The function has the below logic for under allocated agents.

data.rs#L216-L222

            // Distribute to under-allocated agents
            // Weighted by agent imbalance
            let phase1_amount = if imbalances[i] < 0 {
                phase1 * (-imbalances[i] as u128) / neg_diff ---- @@ audit check for `imbalances`
            } else {
                0
            };

The imbalances is calculated in get_weight_imbalances as explained in this issue submission.

data.rs#L174-L182

let diff = staked_amount_current - staked_amount_optimal; ---->>> @@audit check
            if diff > 0 {
                pos_diff += diff as u128;
            } else if diff < 0 {
                neg_diff += -diff as u128;
            }
            total_staked += staked_amount_current as u128;
            stakes.push(staked_amount_current as u128);
            imbalances.push(diff); ---- @@@ audit find - inserted here.

if staked_amount_current is zero, entire staked_amount_optimal is assigned as diff.

Now, lets see the first three cases where the staked_amount_current is zero

when an agent is added - no issue.

When agent un-bond fully from the pool and When agent is slashed fully.. -- in this case , the admin could expect to remove this agent.

If we see the stake function, so, it is difficult to check whether the agent has stake something or not.

Any call to stake just before removing the agent would lead to call_deposit .

data.rs#L257-L267

        // Deposit
        for (i, a) in agents.iter().enumerate() {
            let deposit_amount = deposit_amounts[i];
            if deposit_amount > 0 {
                debug_println!("Depositing {} into agent #{}", deposit_amount, i);
                if let Err(e) = call_deposit(a.address, deposit_amount) {
                    return Err(VaultError::InternalError(e));
                }
            }
        }

if the agent is removed, these funds will not be accounted as staked for user. So, user would be losing portion of their funds.

In the current design, it is difficult to resolve this issue, since the stake function can be called by anyone.

solution

we think of the following solution to prevent users stake.

1. pause the stake function call and remove the agent.

[0xmahdirostami]

When agent un-bond fully from the pool and When agent is slashed fully.. -- in this case , the admin could expect to remove this agent.

if the owner wants to remove an agent, the owner first sets the weight of that agent to 0, so staked_amount_optimal will be 0.

i don't see any issue here, besides that sponsors will review again and if they find any issue, they will comment here.

[aktech297]

Hi Ser,

The sequence of operation matters here. since the stake function is public, anybody can make a call at any time. it could after full slash or unbound and just before the admin set the weight to zero.

one more point to note is, there were no check to verify whether the agent has staked amount or not before setting the weight to zero.

it would be fine if the agent is good. if so, they can be added again. But other point to consider is, what if they are malicious and the admin wanted to remove them and never wanted to add again.

So, there are more number of concerns on this area.

From our side, we would suggest the following to improve the current design .

1. check an agent has staked or not, before removing them. ( could be simple to implement)
2. pause the stake function before removing agent. ( need to incorporate the pause /un-pause logic.) some work involved, but worth to consider.

[bgibers]

When agent un-bond fully from the pool and When agent is slashed fully.. -- in this case , the admin could expect to remove this agent.

if the owner wants to remove an agent, the owner first sets the weight of that agent to 0, so staked_amount_optimal will be 0.

i don't see any issue here, besides that sponsors will review again and if they find any issue, they will comment here.

This is the correct workflow for Kintsu and @0xmahdirostami is right that there shouldn't be an issue here.

one more point to note is, there were no check to verify whether the agent has staked amount or not before setting the weight to zero.

If you think you found another issue please submit it and i'll review accordingly 🙂