dimah7 - Security considerations of ERC6909 are not complied, thus an operator can steal funds

[sherlock-admin3]

dimah7

High

Security considerations of ERC6909 are not complied, thus an operator can steal funds

Summary

The protocol's pools are strictly compilant with ERC6909, which introduces the operator model. An operator is an account which is granted permission to transfer assets on behalf of the owner.

Also according to ERC6909 specification security considerations:

1. The first consideration is consistent with all delegated permission models. Any account with operator permissions may transfer any amount of any token id on behalf of the owner until the operator permission is revoked
2. The second consideration is unique to systems with both delegated permission models. In accordance with the `transferFrom`In accordance with the transferFrom method method, spenders with operator permission are not subject to allowance restrictions, spenders with infinite approvals SHOULD NOT have their allowance deducted on delegated transfers

However these security considerations are not taken of concern. This allows an operator to transfer all the available token balance of the owner to himself. And since for this contest, it's confirmed that only the owners of pools, super pools and position manager are considered as TRUSTED, the operator role is not an owner i consider this scenario likely to happen.

Vulnerability Detail

The problem lies in the Pool::withdraw() function:

function withdraw(uint256 poolId, uint256 assets, address receiver, address owner) public returns (uint256 shares) {
        ...
        if (msg.sender != owner && !isOperator[owner][msg.sender]) {
            uint256 allowed = allowance[owner][msg.sender][poolId];
            if (allowed != type(uint256).max) allowance[owner][msg.sender][poolId] = allowed - shares;
        }
        ...

@>      IERC20(pool.asset).safeTransfer(receiver, assets);

As can be seen the caller can specify any address as the receiver.

Impact

• Impact: High, the entire balance of the owner can be drained
• Likelihood: Medium, because:
  • 1. an owner can revoke the operator role anytime, so an operator can frontrun such transactions to prevent, but some chains have private mempools, so this only partially mitigates the likelihood
  • 2. since by spec, the operator is granted infinite approval, the attacker needs only one successful tx to steal the tokens
• Overall: High/Medium -> High

Code Snippet

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/0b472f4bffdb2c7432a5d21f1636139cc01561a5/protocol-v2/src/Pool.sol#L371

Tool used

Manual Review

Recommendation

Restrict the operator to be able to transfer to himself:

if (receiver == msg.sender && msg.sender != owner) revert("Operator cannot transfer to themselves");

But since he can choose any address, this exploit is not fully mitigated, consider if having an operator for the pools is in need.

[iamnmt]

Escalate

Invalid.

The allowance, and the operator are trusted by the user. If the allowance, or the operator acts maliciously, then it is a user mistake for approving such address.

If this issue is a valid issue then there is no way to mitigate it.

May I ask where is this quoted from?

In accordance with thetransferFromIn accordance with the transferFrom method method, spenders with operator permission are not subject to allowance restrictions, spenders with infinite approvals SHOULD NOT have their allowance deducted on delegated transfers

[sherlock-admin3]

Escalate

Invalid.

The allowance, and the operator are trusted by the user. If the allowance, or the operator acts maliciously, then it is a user mistake for approving such address.

If this issue is a valid issue then there is no way to mitigate it.

May I ask where is this quoted from?

In accordance with thetransferFromIn accordance with the transferFrom method method, spenders with operator permission are not subject to allowance restrictions, spenders with infinite approvals SHOULD NOT have their allowance deducted on delegated transfers

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.

[Blngogo]

Who said that the operator is trusted by default? Any account can rogue at anytime and there must be logic preventing that. Nowhere was mentioned that the operator is TRUSTED entity, nor in the docs, README, ERC6909, the codebase or even the discord channel, there was a question asked about the trusted roles and only pool owners were confirmed by the sponsor. If an operator acts maliciously, this situation can't be considered as user mistake, by this logic almost every reported bug can be considered as owner or user mistake, this is flaw in the code allowing to do that. The root of this issue is coming from the receiver parameter which allows any address to be specified as receiver, which is a freedom given from the devs.

To answer the question of the escalation boss above as i see that he tries to invalidate most of the issues, because he got few uniques maybe, the quoted paragraph was taken from the ERC6909, but it seems that it has changed, notice that it has the Draft tag which means the text can also undergo changes (next time will provide screenshot to clear all doubts). But anyways even with different text the context remains the same.

This report is about the security considerations of ERC6909, which the project insists that strictly implements it (the ERC6909). And overall the erc is about multi-token interface and the operator role is optional service, so it can't be assumed that should be trusted by default. That's why there is security considerations section, warning that the operator has unlimited allowance and such situations as described in the report can happen and there must be actions preventing it.

And for the fix, the issue can be fixed this way: since the job of the operator is to withdraw on behalf of owner, he can be restricted to transfer to himself and address different from the owner. And if the flexibility to specify a receiver address wants to be kept by the project, simply add a check, only if the owner is the msg.sender then you can specify a receiver address.

[cvetanovv]

I agree with the escalation.

When it is not mentioned anywhere whether the external admin (in this case, Operator) is trusted or not trusted, we assume by default that it is TRUSTED. This means that the Operator will not act maliciously.

It is noted in the Sherlock documentation:

(External) Admin trust assumptions: If no values are provided, the (external) admin is trusted to use values that will not cause any issues.

Planning to accept the escalation and invalidate the issue.

[Blngogo]

@cvetanovv Thanks for the valuable feedback sir! However if we look further in the rule that you point out (5. (External) Admin trust assumptions:), there is also said:

"If no values are provided, the (external) admin is trusted to use values that will not cause any issues.

Note: if the attack path is possible with any possible value, it will be a valid issue."

In this case the attack is possible with every possible value, because the operator can specify any address he wants. This can be second account that he holds or an arbitrary address. The final result will be loss of funds for the owner.

Implementing the forementioned recommendation would fix this possible attack: since the job of the operator is to withdraw on behalf of owner, he can be restricted to transfer to himself and address different from the owner. And if the flexibility to specify a receiver address wants to be kept by the project, simply add a check, only if the owner is the msg.sender then you can specify a receiver address.

[cvetanovv]

@Blngogo, You misunderstood the rule. This Note is if the admin is not trusted.

No one Watson in this contest has looked for issues related to external admin because they know it is TRUSTED and such issues are invalid. It would be unfair to all participants to make an exception for this issue and here to recognize external admin as malicious.

My decision to accept escalation remains.

[Blngogo]

@cvetanovv all respect sir, but literally on the above comment you are stating that the admin is TRUSTED based on a Sherlock rule, quoting you:

It is noted in the Sherlock documentation:

(External) Admin trust assumptions: If no values are provided, the (external) admin is trusted to use values that will not cause any issues.

I've read the rules many times in detail, before submitting this report. I agree that the admin is trusted, but the rule for trusted entities has a note clearly stating:

Note: if the attack path is possible with any possible value, it will be a valid issue."

Now suddenly by the rule that you determined the operator as TRUSTED, a paragraph from the same rule applies for UNTRUSTED role? There seems to be a twist in your arguments. If the approved role is not trusted, by this logic there is no point have the note: "if the attack path is possible with any possible value, it will be a valid issue.", since any value that the untrusted operator uses will be crutial. Thats why its mentioned that if a trusted entity can execute the attack with ANY POSSIBLE VALUE, by Sherlock rules it should be a valid issue.

It will be unfair this decision to overwrite the Sherlock rules. By the rules that we must strictly follow, it's expected decisions to be made on them, not on how other participants will feel. Moreover based on your statements seems that, if this issue had dups it would be accepted. Can provide dozens of reports accepted in this platform only because of the rule set. By Sherlock rules, this report should be valid MEDIUM

[samuraii77]

That issue is clearly invalid, even the specs mentioned in the report are not broken:

The first consideration is consistent with all delegated permission models. Any account with operator permissions may transfer any amount of any token id on behalf of the owner until the operator permission is revoked

That's exactly what happens here, nothing broken. The operator can transfer any amount of any token id on behalf of the owner until his permissions are revoked.

The second consideration is unique to systems with both delegated permission models. In accordance with the transferFromIn accordance with the transferFrom method method, spenders with operator permission are not subject to allowance restrictions, spenders with infinite approvals SHOULD NOT have their allowance deducted on delegated transfers

Not sure where this even comes from and it's written incorrectly with multiple mistakes in terms of grammar but let's assume it is written in the specification, it says that spenders with infinite approvals should not have their allowance deducted. That's exactly what happens:

 if (msg.sender != owner && !isOperator[owner][msg.sender]) {
            uint256 allowed = allowance[owner][msg.sender][poolId];
            if (allowed != type(uint256).max) allowance[owner][msg.sender][poolId] = allowed - shares;
        }

If it's the operator, we don't check the allowanwce, if we have an infinite approval, we don't deduct the allowance. Everything is followed.

[Blngogo]

@samuraii77 basically you are pointing out how the protocol works, yes nothing is broken in the code snippet you provided. Read carefully the report, where the impact is the focus. Don't get distracted by the title it's just to point out that the approved role has unlimited balance, pay attention to the root cause, which is the receiver parameter which an operator can choose to transfer assets to. And a Sherlock rule for TRUSTED external admins (as even the HoJ confirmed above) has an important note that in case the attack can be feasible with any provided value (in this case address) it should be a VALID issue.

Please don't try to change the discussions and the root of the report in other direction.

[Nihavent]

The report fails to acknowledge that the owner is the one who sets the operator. Setting an operator that you don't trust is one of an endless number of user mistakes that are not considered valid security issues.

You also say the root cause is that the receiver parameter can be the operator's address, but even with this fixed the malicious operator would send the funds to another EOA they control. So this issue is unavoidable, and the only way to prevent it is to set operators you trust.

[cvetanovv]

This issue is obviously invalid because the operator is an external Admin, and he is TRUSTED by default.

The external Admin will not be trusted if this is explicitly mentioned in the Readme. Since nothing is mentioned there, we assume that the operator is trusted and will not do malicious things.

Furthermore, as @Nihavent mentions, the owner is the one who gives him rights, and he is also TRUSTED to make the right decisions.

From there on, we don't even need to look at whether or not he can do malicious things because we know it is TRUSTED and he won't.

My decision to reject the escalation remains.

[Blngogo]

@cvetanovv Whats your opinion about this part of the rule: "Note: if the attack path is possible with any possible value, it will be a valid issue.". I already mentioned it like 3 times, please don't skip it and would appreciate if you read my comments carefully. Like i said i agree that is TRUSTED. But the rule, you mentioned (about external admins) is for TRUSTED roles correct? You confirmed this. So the part with the note can't be suddenly appliying for UNTRUSTED roles, as there will be a contradiction. However this particular rule very clearly says, if the attack is possible with any possible value it should be valid by RULES. Judge's decisions should NOT override the rules. Rejecting the report, means a judge's decision is above the rules, which is not how this platform works. I'm insisting that this report should be valid, not because the operator should be considered as untrusted, but because of the note in the external admin rule.

@Nihavent i don't have to mention the owner word explicitly in the report, it's understandable from the description. When an account approves a second account to operate on his behalf, usually he is interpreted as "owner or admin", but i assume you know this very well sir.

The fix is: since the job of the operator is to withdraw on behalf of owner, he can be restricted to transfer to himself and address different from the owner. And if the flexibility to specify a receiver address wants to be kept by the project, simply add a check: only IF the owner is the msg.sender then you can specify a receiver address.

[cvetanovv]

@cvetanovv Whats your opinion about this part of the rule: "Note: if the attack path is possible with any possible value, it will be a valid issue.".

This only applies if the admin is restricted.

Moreover, the protocol did not specify any values for external admin: https://github.com/sherlock-audit/2024-08-sentiment-v2?tab=readme-ov-file#q-are-there-any-limitations-on-values-set-by-admins-or-other-roles-in-protocols-you-integrate-with-including-restrictions-on-array-lengths

only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.

The judge's decisions should NOT override the rules. If no such value is specified, it cannot be a valid issue.

My decision to reject the escalation remains.

[ruvaag]

if the issue here is that once a depositor / admin assigns someone as an operator, the operator can do whatever they want with the depositors shares... this is intended.

rationale: (1) admin is trusted (2) designation of an operator is parallel to multiple ERC20 approvals, user error cannot be a valid issue

[Blngogo]

@cvetanovv So essentially all the statements from the external admin rule applies for trusted roles, but exactly this paragraph with the note applies for restricted roles? Where was this mentioned? What's this kind of twist in the rule set? Seems a little inappropriate, how could a watson differentiate, that the note part applies exactly for untrusted roles? The README, doesn't mention any value, so that essentially should mean any value. I get that quoted part:

only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.

But exactly after this comes the note: if the attack is possible with any value, it should be valid. Which makes sense. If this report had 20+ dups quoting the same paragraph, this would be valid. Maybe it would be better, additional clarification to be added in that particular rule or generally the note part to be removed, because it makes a small door, reports like this to have a reason to be validated.

You just said that you are identifying the operator as trusted based on the external admin rule, so you can't now state that one paragraph from the same rule applies for restricted roles, this is totally unfair. It's clear how the operators should execute, but this note allows this report's rejection to be bypassed in my opinion.

[Evert0x]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• iamnmt: accepted