n33k

high

_chargeFee in MultiInvoker could be an unwitting accomplice of fishing attack

Summary

_chargeFee is granted excessive abilities, which could be exploited by malicious user interface to steal from users with the help of the MultiInvoker contract.

Vulnerability Detail

The _chargeFee function inside MultiInvoker should only be able to take a portion of user's trading values as the interface fee. While the implementation allows to take over all the user approved USDC.

function _chargeFee(address receiver, UFixed18 amount, bool wrapped) internal {
    if (wrapped) {
        USDC.pull(msg.sender, amount);
        _handleWrap(receiver, amount);
    } else {
        USDC.pullTo(msg.sender, receiver, amount);
    }
}

Impact

Malicious website can steal users with various methods, they can trick users to sign a malicious message, approve or call a malicious contract, etc. Users will usually notice these fishing behaviors when wallet promots them to sign a transaction that approves or calls a suspicious contract address.

Users who tend to interact with this project will trust and sign the transactions that approve and call the MultiInvoker contract address. However, if they are interacting with a malicious interface, the interface can set the amount parameter to the approved USDC amount to steal users. Users usually won't notice this abnormal bytes in the calldata and some wallets even don't decode and show the calldata to users.

The malicious interface's tracks are not stored on chain so he can escape easily after the crime and leave this project to bear the blame. This could severely damage the project's reputation and might lead to legal complications.

Code Snippet

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/multiinvoker/MultiInvoker.sol#L385-L392

Tool used

Manual Review

Recommendation

Record the total trading amount in the MultiInvoker actions.
Ensure _chargeFee is the last action and only take a restricted portion of the total trading amount.

Escalate for 10 USDC.

Quote from lead judge:

chargeFee pulls funds from msg.sender. If the msg.sender has sent a tx that deposits money to another account, it is no different than him simply doing it via any other method...

The judge misunderstood this issue. The tx was not built by msg.sender, but by the user interface/web frontend. The msg.sender only signed the tx. This issue demonstrates a senario where msg.sender is tricked into signing a malicious tx built by 3rd party frontend(a new role introduced in this protocol).

This is a unique vulnerability in a unique feature of this protocol which was not present in other audits we normally saw.

The attack vector is also quite different in that it's not about writing a smart contract exploit to exploit a smart contract bug.

I will elaborate more to help understanding this issue.

First, we need to understand the purpose of this `_chargeFee` function.

In the comment of chargeFee function, it says:

Helper function to include an interface fee

So what is interface?

interface should be user interface, which including the web interface that user interacts with in the browser.

What is the purpose of this interface fee?

It's to incentivize the community to build 3rd party user interfaces around these smart contracts of the protocol. This is a unique feature of this protocol.

This introduced a new UNTRUSTED USER role to this protocol: interface.

Second, let's understand the vulnerability in `_chargeFee` function.

Since a new role is introduced, we need to assess the security impact of this role.

In _chargeFee function, it allows the interface to charge fee from the users who use this interface.

interface fee should be only a small portion of the user's trading values.

_chargeFee gives excessive power to the interface to take all the approved amount from the user.

    function _chargeFee(address receiver, UFixed18 amount, bool wrapped) internal {
        if (wrapped) {
            USDC.pull(msg.sender, amount);
            _handleWrap(receiver, amount);
        } else {
            USDC.pullTo(msg.sender, receiver, amount);
        }
    }

Let's see how this vulnerability could be exploited.

The attacker first builds a legitimate interface, and spreads it in the community to attract users.

When user interacts with the smart contract through this interface, it is the interface who builds the transaction payload.

In the early days, the interface builds legitimate tx payloads which only take small portion interface fee for the user to sign.

One day the interface turns malicious. When a user is trading using this interface, the interface builds a tx payload which takes all the approved amount from the user.

Clarification on some questions

Q: Since the user is tricked into using the malicious web interface, the interface can construct various malicious TXs for user to sign. Why bother taking advantage of this _chargeFee function?

A: The wallet(eg. metamask) will decode the tx payload and show the smart contract address that the user is interacting with. If the user is interacting with a suspicious smart contract or is transferring funds to a suspicious address, the user will notice it and will not sign the tx. This will effectively block the attack. But if the wallet shows that the user is interacting with MultiInvoker, the user will not suspect the malicious behaviour and will sign the tx.

Q: Is protocol like Uniswap vulnerable to this attack? Since web interface can also build malicious tx payload arround Uniswap router.

A: No. Because Uniswap doesn't have this interface fee incenitives. It doesn't encourage 3rd party interfaces and some protocols explicitly set a warning to user of using web interfaces. Here's a waning in the home page of PancakeSwap.

PHISHING WARNING:
please make sure you're visiting
https://pancakeswap.finance
- check the URL carefully.

Q: Is this issue in the scope of sherlock audit?

A: The vulnerability lines inside MultiInvoker.sol and the impact is that an UNTRUSTED ROLE(interface) can rug pull other users who are using the protocol in a correct manner(i.e. trading using a 3rd party interface). So I thinks this should be a valid High in this audit.

Q: What is the right way to do?

A: First limit the amount of fee that the interface can charge.

Second, it is better to make an aggrement with users of using 3rd party interface on the contract level. For example, design a function called approveInterface which takes the interface address as parameter. When user signs a tx call to this function, the user is aware of the risk of using this interface and the fee only goes to this approved interface. This will exempt the project from the responsibility of the malicious behaviour of the interface to some extent.

Escalate for 10 USDC.

Quote from lead judge:

chargeFee pulls funds from msg.sender. If the msg.sender has sent a tx that deposits money to another account, it is no different than him simply doing it via any other method...

The judge misunderstood this issue. The tx was not built by msg.sender, but by the user interface/web frontend. The msg.sender only signed the tx. This issue demonstrates a senario where msg.sender is tricked into signing a malicious tx built by 3rd party frontend(a new role introduced in this protocol).

This is a unique vulnerability in a unique feature of this protocol which was not present in other audits we normally saw.

The attack vector is also quite different in that it's not about writing a smart contract exploit to exploit a smart contract bug.

I will elaborate more to help understanding this issue.

First, we need to understand the purpose of this _chargeFee function.

In the comment of chargeFee function, it says:

Helper function to include an interface fee

So what is interface?

interface should be user interface, which including the web interface that user interacts with in the browser.

What is the purpose of this interface fee?

It's to incentivize the community to build 3rd party user interfaces around these smart contracts of the protocol. This is a unique feature of this protocol.

This introduced a new UNTRUSTED USER role to this protocol: interface.

Second, let's understand the vulnerability in _chargeFee function.

Since a new role is introduced, we need to assess the security impact of this role.

In _chargeFee function, it allows the interface to charge fee from the users who use this interface.

interface fee should be only a small portion of the user's trading values.

_chargeFee gives excessive power to the interface to take all the approved amount from the user.
    function _chargeFee(address receiver, UFixed18 amount, bool wrapped) internal {
        if (wrapped) {
            USDC.pull(msg.sender, amount);
            _handleWrap(receiver, amount);
        } else {
            USDC.pullTo(msg.sender, receiver, amount);
        }
    }
Let's see how this vulnerability could be exploited.

The attacker first builds a legitimate interface, and spreads it in the community to attract users.

When user interacts with the smart contract through this interface, it is the interface who builds the transaction payload.

In the early days, the interface builds legitimate tx payloads which only take small portion interface fee for the user to sign.

One day the interface turns malicious. When a user is trading using this interface, the interface builds a tx payload which takes all the approved amount from the user.

Clarification on some questions

Q: Since the user is tricked into using the malicious web interface, the interface can construct various malicious TXs for user to sign. Why bother taking advantage of this _chargeFee function?

A: The wallet(eg. metamask) will decode the tx payload and show the smart contract address that the user is interacting with. If the user is interacting with a suspicious smart contract or is transferring funds to a suspicious address, the user will notice it and will not sign the tx. This will effectively block the attack. But if the wallet shows that the user is interacting with MultiInvoker, the user will not suspect the malicious behaviour and will sign the tx.

Q: Is protocol like Uniswap vulnerable to this attack? Since web interface can also build malicious tx payload arround Uniswap router.

A: No. Because Uniswap doesn't have this interface fee incenitives. It doesn't encourage 3rd party interfaces and some protocols explicitly set a warning to user of using web interfaces. Here's a waning in the home page of PancakeSwap.
PHISHING WARNING:
please make sure you're visiting
https://pancakeswap.finance
- check the URL carefully.
Q: Is this issue in the scope of sherlock audit?

A: The vulnerability lines inside MultiInvoker.sol and the impact is that an UNTRUSTED ROLE(interface) can rug pull other users who are using the protocol in a correct manner(i.e. trading using a 3rd party interface). So I thinks this should be a valid High in this audit.

Q: What is the right way to do?

A: First limit the amount of fee that the interface can charge.

Second, it is better to make an aggrement with users of using 3rd party interface on the contract level. For example, design a function called approveInterface which takes the interface address as parameter. When user signs a tx call to this function, the user is aware of the risk of using this interface and the fee only goes to this approved interface. This will exempt the project from the responsibility of the malicious behaviour of the interface to some extent.

You've created a valid escalation for 10 USDC!

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.

Hi @syjcnss , I think that where we differ is in the assumption that the interfacer is an untrusted user and that that is Perennial's responsibility. Once you assume that the interface (a 3rd-party) is malicious, the problem will not be the chargeFee function; the problem will be that the user is signing transactions that will harm him.

For example, let's say we accept the submission and put protections on chargeFee... The malicious interface can still call MultiInvoker, initiate invoke and deposit user's funds to the interface account, or withdraw user funds to the interface account... So you see, the issue is not chargeFee, the issue is that the user is issuing transactions from a malicious 3rd-party source. This is beyond Perennial's reach. Therefore I still think the issue is invalid.

Understand your point.

For example, let's say we accept the submission and put protections on chargeFee... The malicious interface can still call MultiInvoker, initiate invoke and deposit user's funds to the interface account, or withdraw user funds to the interface account...

I've also thought about this. Maybe introducing such role intrudced a new attack interface?

I'm not sure. Want to hear the sponsor's thought on this.

This is either a frontend bug (malicious user interface) or a user error (inputting incorrect parameter), and neither should result in a valid issue. @syjcnss

Result: Invalid Unique See previous message.

Escalations have been resolved successfully!

Escalation status:

syjcnss: rejected

sherlock-audit / 2023-05-perennial-judging

n33k - _chargeFee in MultiInvoker could be an unwitting accomplice of fishing attack #154

_chargeFee in MultiInvoker could be an unwitting accomplice of fishing attack

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

First, we need to understand the purpose of this `_chargeFee` function.

Second, let's understand the vulnerability in `_chargeFee` function.

Let's see how this vulnerability could be exploited.

Clarification on some questions

First, we need to understand the purpose of this `_chargeFee` function.

Second, let's understand the vulnerability in `_chargeFee` function.

Let's see how this vulnerability could be exploited.

Clarification on some questions

sherlock-audit / 2023-05-perennial-judging

n33k - _chargeFee in MultiInvoker could be an unwitting accomplice of fishing attack #154

_chargeFee in MultiInvoker could be an unwitting accomplice of fishing attack

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

First, we need to understand the purpose of this _chargeFee function.

Second, let's understand the vulnerability in _chargeFee function.

Let's see how this vulnerability could be exploited.

Clarification on some questions

First, we need to understand the purpose of this _chargeFee function.

Second, let's understand the vulnerability in _chargeFee function.

Let's see how this vulnerability could be exploited.

Clarification on some questions

First, we need to understand the purpose of this `_chargeFee` function.

Second, let's understand the vulnerability in `_chargeFee` function.

First, we need to understand the purpose of this `_chargeFee` function.

Second, let's understand the vulnerability in `_chargeFee` function.