ProofOfHumanity : `voucherHumanity` who vouched for other could be expired when processVouches

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x14775ea116f1dfe2ade3c5b09e0277ba8f037943ae974afb5adedd70ed5c4682 Severity: medium

Description: Description\

An humanity can vouch for other user's request. This can be done inside the function advanceState.

ProofOfHumanity.sol#L957-L973

            if (isValid) {
                // If voucherAccount is null, voucherHumanityId will be null too which cannot be claimed (so it will fail the conditions)
                bytes20 voucherHumanityId = accountHumanity[voucherAccount];
                voucherHumanity = humanityData[voucherHumanityId];
                if (
                    voucherHumanity.owner == voucherAccount &&
                    block.timestamp < voucherHumanity.expirationTime --->> expiry check done here 
                    &&
                    !voucherHumanity.vouching &&
                    voucherAccount != _claimer
                ) {
                    request.vouches.push(voucherHumanityId);
                    voucherHumanity.vouching = true;

                    // Emit event to signal the processing of the signature vouch (and the on-chain as well)
                    emit VouchRegistered(voucherHumanityId, humanityId, requestId);
                }
            }

after this the request is marked as Status.Resolving and the challengePeriodStart is initizated.

Once the challenge period is past, the user can call the executeRequest function to claim the humanity.

Where the vouches is traversed by calling the function processVouches and the voucher status is updated. After that the reward is transferred.

inside the function processVouches the lifetime of the humanity who vouced for this request is not checked. They might have expired life.

The problem here is, the humanity who vouched could have expired within the challenge window.

Attack Scenario\

An expired humanity's vouch could be used to process the user request to claim the humanity.

Attachments

2. Revised Code File (Optional)

when advanceState, update the check as shown below.

ProofOfHumanity.sol#L957-L973

            if (isValid) {
                // If voucherAccount is null, voucherHumanityId will be null too which cannot be claimed (so it will fail the conditions)
                bytes20 voucherHumanityId = accountHumanity[voucherAccount];
                voucherHumanity = humanityData[voucherHumanityId];
                if (
                    voucherHumanity.owner == voucherAccount &&
                    block.timestamp < voucherHumanity.expirationTime &&
                    voucherHumanity.expirationTime > (block.timestamp + challengePeriodDuration) && --->>> updated check
                    !voucherHumanity.vouching &&
                    voucherAccount != _claimer
                ) {
                    request.vouches.push(voucherHumanityId);
                    voucherHumanity.vouching = true;

                    // Emit event to signal the processing of the signature vouch (and the on-chain as well)
                    emit VouchRegistered(voucherHumanityId, humanityId, requestId);
                }
            }

This will make sure that the voucher can be still active.

[clesaege]

The problem here is, the humanity who vouched could have expired within the challenge window.

This is not a problem, in case the vouch was malicious, the voucher will be removed, but if it is already expired, this wouldn't have any effect.

[aktech297]

The problem here is, the humanity who vouched could have expired within the challenge window.

This is not a problem, in case the vouch was malicious, the voucher will be removed, but if it is already expired, this wouldn't have any effect.

@clesaege are you sure you understood the issue ?

you are referring the apply penalty. This is not the case. The vouch does not need to be malicious here. As per humanity, an expired humanity does not have any value.

Person A vouch. Humanity of A expired in the challenge period. when processing the vouch request, it is counted.

Can you check this issue once again.

[aktech297]

The problem here is, the humanity who vouched could have expired within the challenge window.

This is not a problem, in case the vouch was malicious, the voucher will be removed, but if it is already expired, this wouldn't have any effect.

Hi @clesaege lets me explain the cases clearly.

The issue is talking about the vouches that is going to be expired soon after vouch for others in the challenge window.

Lets see with following example:

• Accounts A, B and C have the humanity of their to expire at 10 mins from now.
• All three vouches for Alice and submits the signature.
• lets assume the challenge window is 30 mins.
• Alice gets the vouch signature and gets the attestation (request.vouches.push array will be updated and the while loop completes)
• After 10 mins, A, B and C humanity expired.
• Now, challenge period passed with or without challenge.
• Alice calls the executeRequest and gets the humanity.

As we mentioned, the current fix is in the function . when updating the request.vouches.push( in advanceState function, add the below mentioned check.

            if (
                voucherHumanity.owner == voucherAccount &&
                block.timestamp < voucherHumanity.expirationTime &&

      voucherHumanity.expirationTime > (block.timestamp + challengePeriodDuration) && --->>> updated check

!voucherHumanity.vouching && voucherAccount != _claimer ) {

[clesaege]

Lets see with following example:

• Accounts A, B and C have the humanity of their to expire at 10 mins from now.
• All three vouches for Alice and submits the signature.
• lets assume the challenge window is 30 mins.
• Alice gets the vouch signature and gets the attestation (request.vouches.push array will be updated and the while loop completes)
• After 10 mins, A, B and C humanity expired.
• Now, challenge period passed with or without challenge.
• Alice calls the executeRequest and gets the humanity.

I don't see anything wrong with this example, the validity of vouches is checked when calling advanceState. They don't need to be valid for the whole challenge period.

[aktech297]

Lets see with following example:

• Accounts A, B and C have the humanity of their to expire at 10 mins from now.
• All three vouches for Alice and submits the signature.
• lets assume the challenge window is 30 mins.
• Alice gets the vouch signature and gets the attestation (request.vouches.push array will be updated and the while loop completes)
• After 10 mins, A, B and C humanity expired.
• Now, challenge period passed with or without challenge.
• Alice calls the executeRequest and gets the humanity.

I don't see anything wrong with this example, the validity of vouches is checked when calling advanceState. They don't need to be valid for the whole challenge period.

Hi @clesaege are you sure with this behaviour ?

We don't see any reason for having the expiry check then in advanceState function.

Lets see the following case.

Alice humanity is going to be expired in next 60 seconds. Alice vouch for Bob who is malicious. Bob use this vouch with in this 60 seconds. Alice humanity is expired. Malicious bob can use this vocuh to claim the humanity.

The other case is, Alice could turn into malicious. Bob uses Alice's vouch. Alice humanity expired. Alice can not be penalized as their humanity is expired.

In both cases, the processvouch is not checking the life of the vouchedHumanity.

If we see the other parts of code, atleast considerable amount of time is used to process in such edge cases. For example in fundAppeal function, 50% of the appeal period is considered to decide the loserStakeMultiplier

fundAppeal

        Party winner = Party(IArbitrator(_arbitrator).currentRuling(_disputeId));
        if (winner == _side) multiplier = winnerStakeMultiplier;
        else if (winner == Party.None) multiplier = sharedStakeMultiplier;
        else if (block.timestamp - appealPeriodStart < (appealPeriodEnd - appealPeriodStart) / 2)
            multiplier = loserStakeMultiplier;
            // If half of appeal period passed and side funded is winner, it will revert
        else revert();

Similarly, advanceState can have the time threshold.

[clesaege]

For now, you've only described expected behaviours. If you are unregistered by the time of penalization (no matter the reason), you cannot be penalized. People who are removed via penalization can always resubmit. The penalization mechanism is there to remove people vouching for malicious profiles as those are likely to be malicious themselves.

[aktech297]

For now, you've only described expected behaviours. If you are unregistered by the time of penalization (no matter the reason), you cannot be penalized. People who are removed via penalization can always resubmit. The penalization mechanism is there to remove people vouching for malicious profiles as those are likely to be malicious themselves.

Why do you need to have the expiry check then? Its not about Challenging always. People have other works to do instead of complaining . Anyway its upto you who state behaviour and issue at last.

Thanks

[clesaege]

Why do you need to have the expiry check then?

To check if the profile is registered, only registered profiles can vouch.

[aktech297]

Why do you need to have the expiry check then?

To check if the profile is registered, only registered profiles can vouch.

If owner is valid that is sufficient .

[clesaege]

No, because the submission could be expired.

[aktech297]

No, because the submission could be expired.

that is not done when processing the vouch. expiry check is done only when getting the vouch. by the time, it completes the challenge period, the voucher already expired. imo, this design need to be re-looked.

my intention here is not for the prize. It about the validity of the current system design. Allowing someone to vouch who is going expire soon after the vouch is fundamental flaw.

I am not sure why such design is allowed.

Do you have any reason for making it as expected behavior ?

[clesaege]

that is not done when processing the vouch. expiry check is done only when getting the vouch. by the time, it completes the challenge period, the voucher already expired.

Yeah this is the expected behaviour.

Do you have any reason for making it as expected behavior ?

There is no point in removing someone who has already been removed. From what I understand what you propose is to make vouches invalid challengePeriodDuration time before the profile expires. This would result in breaking the logic of "Valid profiles can count as vouches for one profile at a time" by adding an exception that they are not valid challengePeriodDuration before expirationTime. Even then, a submission could take the full theoretical length of the dispute period which can (in theory) take a couple of months. So if you wanted to prevent expired profile by the time vouches are processed, you'd need to make vouches invalid a couple of months before expirationTime which would be annoying and confusing UX wise.

[aktech297]

that is not done when processing the vouch. expiry check is done only when getting the vouch. by the time, it completes the challenge period, the voucher already expired.

Yeah this is the expected behaviour.

Do you have any reason for making it as expected behavior ?

There is no point in removing someone who has already been removed. From what I understand what you propose is to make vouches invalid challengePeriodDuration time before the profile expires. This would result in breaking the logic of "Valid profiles can count as vouches for one profile at a time" by adding an exception that they are not valid challengePeriodDuration before expirationTime. Even then, a submission could take the full theoretical length of the dispute period which can (in theory) take a couple of months. So if you wanted to prevent expired profile by the time vouches are processed, you'd need to make vouches invalid a couple of months before expirationTime which would be annoying and confusing UX wise.

hey.. my suggestion was not to allow to vouch if someone's humanity is going to expire within the challenge window. It will not give any issues.

[clesaege]

From what I understand what you propose is to make vouches invalid challengePeriodDuration time before the profile expires.

hey.. my suggestion was not to allow to vouch if someone's humanity is going to expire within the challenge window.

Those are equivalent so the points of my previous post stand.

[aktech297]

From what I understand what you propose is to make vouches invalid challengePeriodDuration time before the profile expires.

hey.. my suggestion was not to allow to vouch if someone's humanity is going to expire within the challenge window.

Those are equivalent so the points of my previous post stand.

no it wont. the code is punishing the vouchers when the humanity requester is malicious. but in this design what we are discussing, it can not happen.

[clesaege]

Yep, because you can't kick out someone of a place he is not.

This will be my final comment.