[METER-LSD-09] BucketUpdateCandidate does not update the candidate's buckets

[kalos-security]

[METER-LSD-09] BucketUpdateCandidate does not update the candidate's buckets

Impact

After the bucket's candidate has been updated to a different candidate, the new candidate's existing buckets/delegators will receive fewer rewards, as the new candidate's total votes (the denominator for calculating shared rewards) will be added.

Description

The following BucketUpdateCandidate code subtracts the bucket's TotalVotes from the original candidate's TotalVotes. It adds it to the new candidate's TotalVotes but does not remove the bucket from the original candidate's bucket list. It does not add the bucket whose candidate is being updated to the new candidate's bucket list.

func (e *MeterTracker) BucketUpdateCandidate(owner meter.Address, id meter.Bytes32, newCandidateAddr meter.Address) error {
    candidateList := e.state.GetCandidateList()
    bucketList := e.state.GetBucketList()

    b := bucketList.Get(id)
    if b == nil {
        return errBucketNotListed
    }

    nc := candidateList.Get(newCandidateAddr)
    if nc == nil {
        return errCandidateNotListed
    }

    c := candidateList.Get(b.Candidate)
    // subtract totalVotes from old candidate
    if c != nil {
        if c.TotalVotes.Cmp(b.TotalVotes) < 0 {
            return errNotEnoughVotes
        }
        c.TotalVotes.Sub(c.TotalVotes, b.TotalVotes)
    }
    // add totalVotes to new candidate
    nc.TotalVotes.Add(nc.TotalVotes, b.TotalVotes)
    b.Candidate = nc.Addr

    e.state.SetBucketList(bucketList)
    e.state.SetCandidateList(candidateList)
    return nil
}

func (s *Staking) calcDelegates(env *setypes.ScriptEnv, bucketList *meter.BucketList, candidateList *meter.CandidateList, inJailList *meter.InJailList) {
    // ...
    for _, c := range candidateList.Candidates {
        delegate := &meter.Delegate{
            Address:     c.Addr,
            PubKey:      c.PubKey,
            Name:        c.Name,
            VotingPower: c.TotalVotes,
            IPAddr:      c.IPAddr,
            Port:        c.Port,
            Commission:  c.Commission,
        }
        // ...

        for _, bucketID := range c.Buckets {
            b := bucketList.Get(bucketID)
            if b == nil {
                s.logger.Info("get bucket from ID failed", "bucketID", bucketID)
                continue
            }
            // amplify 1e09 because unit is shannon (1e09),  votes of bucket / votes of candidate * 1e09
            shares := big.NewInt(1e09)
            shares = shares.Mul(b.TotalVotes, shares)
            shares = shares.Div(shares, c.TotalVotes)
            delegate.DistList = append(delegate.DistList, meter.NewDistributor(b.Owner, b.Autobid, shares.Uint64()))
        }
        delegates = append(delegates, delegate)
    }
       // ...
}

Recommendations

Remove the bucket from the previous candidate's bucket list and add the bucket to the new candidate's bucket list.

[simonzg]

this issue is fixed in commit https://github.com/meterio/meter-pov/commit/47abc0cebea299fb965eb449f7eda29089abb2a9

[kalos-security]

@simonzg TotalVotes should also be updated. If TotalVotes is not updated, the reward calculation will have problems.

[kalos-security]

@simonzg We apologize for the confusion regarding TotalVotes. Upon further review, we have identified that the handling of TotalVotes is indeed done in the AddBucket and SubBucket functions.