Dliteofficial - Absence of penalties for erroneus clients as client could frontrun disapproval, takes out rewards before he could be prevented from doing so

[sherlock-admin2]

Dliteofficial

medium

Absence of penalties for erroneus clients as client could frontrun disapproval, takes out rewards before he could be prevented from doing so

Summary

Vulnerability Detail

When a client register for client participation rewards, the DAO has a responsibility to approve or deny the request. To approve or disapprove, the DAO has to call Rewards::setClientApproval(). This process helps DAO to keep the clients in check and accountable. This makes this functionality really important.

Say a malicious client is flagged by the DAO, they have a responsibility of revoking the approval of the client. Doing this means the client will effectively be unable to withdraw the rewards they have accumulated. This is where front-running comes in. If the malicious client notices that they have been made, and a disapprove call has been sent to contract, he can front run the tx call. This causes issues for the protocol because then the malicious client would not exactly be held accountable for any inconsistencies casued by them.

Impact

Lack of accountability and repercussion on the part on the DAO to the client. The client can still withdraw the rewards he has garnered over time

Code Snippet

    function setClientApproval(uint32 clientId, bool approved) public onlyOwner {
        RewardsStorage storage $ = _getRewardsStorage();
        $._clientMetadata[clientId].approved = approved;
        emit ClientApprovalSet(clientId, approved);
    }

Tool used

Manual Review

Recommendation

Make the client rewards withdrawal process a timelock where withdrawals can only be made after a certain time has passed. Plus, any client can be removed with this delay period. This way the malicious actor will be unable to make immediate withdraw.

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

WangAudit commented:

I believe it's a low but a very good catch; as I understand; the rewards will be assigned to those users and even if the approval is revoked the mrewards cannot be taken back and they just sit in the contract. Also as I understand they get rewards for facilitating the process; therefore; I don't see what malicious they can do

[Dliteofficial]

escalate

I disagree. I think this is valid and a medium, here's why:

    function withdrawClientBalance(uint32 clientId, address to, uint96 amount) public whenNotPaused {
        RewardsStorage storage $ = _getRewardsStorage();
        ClientMetadata storage md = $._clientMetadata[clientId];

        require(ownerOf(clientId) == msg.sender, 'must be client NFT owner');
        require(md.approved, 'client not approved');

        uint96 withdrawnCache = md.withdrawn;
        require(amount <= md.rewarded - withdrawnCache, 'amount too large');

        md.withdrawn = withdrawnCache + amount;

        emit ClientBalanceWithdrawal(clientId, amount, to);

        $.ethToken.safeTransfer(to, amount);
    }

Looking closely at the above, you'll see why frontrunning disapproval is a serious vulnerability. The issue concerns what happens when a client notices that his approval is about to be revoked. As you have rightly mentioned, the rewards of a client whose approval status has been revoked would sit in the contract, but the client can evade this by calling Rewards::withdrawClientBalance() while he is still approved, which enables him to remove all the rewards he has gathered.

As for the malicious practices by clients, it could be anything like illegally soliciting token holders to vote on a malicious proposal. The reason for disapproval wasn't elaborated upon in the discussion draft and announcements.

[sherlock-admin2]

escalate

I disagree. I think this is valid and a medium, here's why:

    function withdrawClientBalance(uint32 clientId, address to, uint96 amount) public whenNotPaused {
        RewardsStorage storage $ = _getRewardsStorage();
        ClientMetadata storage md = $._clientMetadata[clientId];

        require(ownerOf(clientId) == msg.sender, 'must be client NFT owner');
        require(md.approved, 'client not approved');

        uint96 withdrawnCache = md.withdrawn;
        require(amount <= md.rewarded - withdrawnCache, 'amount too large');

        md.withdrawn = withdrawnCache + amount;

        emit ClientBalanceWithdrawal(clientId, amount, to);

        $.ethToken.safeTransfer(to, amount);
    }

Looking closely at the above, you'll see why frontrunning disapproval is a serious vulnerability. The issue concerns what happens when a client notices that his approval is about to be revoked. As you have rightly mentioned, the rewards of a client whose approval status has been revoked would sit in the contract, but the client can evade this by calling Rewards::withdrawClientBalance() while he is still approved, which enables him to remove all the rewards he has gathered.

As for the malicious practices by clients, it could be anything like illegally soliciting token holders to vote on a malicious proposal. The reason for disapproval wasn't elaborated upon in the discussion draft and announcements.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[WangSecurity]

The problem that I cannot understand, why it is bad that the client, who is going to be disapproved, front runs and takes the rewards that he earned? For me it seems like it's working as intended, can you please elaborate on this part specifically, cause to me it seems low (not invalid completely, just low severity) since no one loses value.

Moreover, if the malicious client understand they will get disapproved at some point, they will redeem all the rewards as soon as they're able to, therefore, I don't see it as a medium severity.

[Dliteofficial]

The DAO Treasury transfers the funds/rewards because the contract doesn't have the rewards to give; instead, a record is kept of how many rewards a client has gathered. For example, if the rewards were transferred close to when a client disapproval proposal happens, then frontrunning becomes a profitable exploit for the client. Hence, even if he knows that disapproval is coming, if no funds are in the contract, this is a no-go.

[WangSecurity]

Then it seems strange why the team will first deposit the rewards and then disapprove a client?

I still think that it works as a bbehaviour and the client actually earned they reward and even if they get disapproved and not front-run it with a withdraw, then the rewards just sit in the contract.

I genuinely don't see it as a harm to the protocol, since they actually facilitated the proposal, therefore, eligible to get the rewards and if they get disapproved and front-run it with a withdraw, I don't see how it harms the protocol. Maybe I'm biased here, but I still believe it's not a significant enough issue to be med.

[Dliteofficial]

I understand your bias, trust me, I do but as stated by the protocol in their announcement content on the client incentives, this functionality exist to reprimand malicious clients, if this penalty can be evaded by a client, I think it should be a med

[WangSecurity]

But the penalty is not avoided. After the disapproval call, the client cannot withdraw any rewards, even tho they can earn them.

[Dliteofficial]

Escalation ends in a couple minutes so I'll just drop this here. The point of attack is between the success of a disapproval proposal and it's execution on a malicious client. If there are rewards in the contract and the malicious client knows the revocation of approval is coming then he can withdraw his rewards before the disapproval proposal is executed. When there is no reward in the contract to withdraw, the disapproval is effective. All rewards for the clients cannot be withdrawn.

[WangSecurity]

What I can add here is that there are two requirements to send out the rewards: number of eligible proposals > numProposalsEnoughForReward OR time since last reward distribution minimumRewardsPeriod. Therefore, I think it's known when the rewards are sent out and it's going to be quite a rare event, therefore, I think there's a higher probability of admins disapproving all the malicious clients upfront.

I understand that this is not a very strong argument, but I still believe that this should indeed remain invalid, since to mee it looks like a potential improvement suggestion.

[eladmallel]

we still think this issue should be excluded, because:

1. we have an admin role that can pause withdrawals (and other functions) quickly (without a proposal process); this account can quickly mitigate the risk being discussed here.
2. even if this behavior can happen, the damage one client can do is quite limited; there is a strong social layer to the DAO that would not continue using a client that was misbehaving in any obvious way, i.e. the probability and impact are both very low.

[cvetanovv]

I agree with the sponsor and the Lead Judge comments. This issue is Low. The impact and probability are Low. In addition, the admin can react immediately and prevent this if necessary.

I'm planning to reject the escalation and leave the issue as it is.

[Evert0x]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• Dliteofficial: rejected