MultiInvoker.sol: Charging interface fee can fail due to inconsistent rounding

Summary

The MultiInvoker contract features a _chargeFee function that allows applications that integrate with MultiInvoker and provide service to other users to charge a fee.

Vulnerability Detail

When the interface fee is charged and wrapped=true (USDC should be wrapped as DSU), then amount USDC is transferred from msg.sender to the MultiInvoker contract:

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

The amount has 18 decimals so it needs to be converted to 6 decimals for calling the actual USDC contract. When calling pull without a flag argument we can see that this defaults to rounding down (false means rounding down): https://github.com/sherlock-audit/2023-05-perennial/blob/main/root/contracts/token/types/Token6.sol#L126

Rounding down in this case is an issue because both the Batcher or the Reserve that are downstream called try to transfer an amount from the MultiInvoker that is rounded UP.

https://github.com/equilibria-xyz/emptyset-batcher/blob/9df117894ba75c0688ca7ff5f70a5a1fd393a2b5/contracts/batcher/Batcher.sol#L63-L66

So the transfer can fail.

E.g. amount=1000000100000000000 would be rounded down to 1000000 but the amount that is tried to be wrapped is 1000001.

We can also see that rounding up needs to occur based on different instances of the same functionality: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/multiinvoker/MultiInvoker.sol#L237-L242

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

Impact

The impact is that those wanting to charge interface fees would not be able to in some cases which is a loss of funds for them.

Code Snippet

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

https://github.com/equilibria-xyz/emptyset-batcher/blob/9df117894ba75c0688ca7ff5f70a5a1fd393a2b5/contracts/batcher/Batcher.sol#L63-L66

Tool used

Manual Review

Recommendation

The issue can be solved by rounding up when converting the amount from 18 decimals to 6 decimals.

diff --git a/perennial-mono/packages/perennial/contracts/multiinvoker/MultiInvoker.sol b/perennial-mono/packages/perennial/contracts/multiinvoker/MultiInvoker.sol
index 8823bea..424ae66 100644
--- a/perennial-mono/packages/perennial/contracts/multiinvoker/MultiInvoker.sol
+++ b/perennial-mono/packages/perennial/contracts/multiinvoker/MultiInvoker.sol
@@ -384,7 +384,7 @@ contract MultiInvoker is IMultiInvoker, UInitializable {
      */
     function _chargeFee(address receiver, UFixed18 amount, bool wrapped) internal {
         if (wrapped) {
-            USDC.pull(msg.sender, amount);
+            USDC.pull(msg.sender, amount,true);
             _handleWrap(receiver, amount);
         } else {
             USDC.pullTo(msg.sender, receiver, amount);

Duplicate of #220

roguereddwarf commented 1 year ago

Escalate for 10 USDC

Disagree with marking this and the parent issue as Low. The loss of funds here is for those that want to charge the interface fee. The transaction charging the interface fee would fail.

Since we can consider those charging the interface fee as users of the protocol (they are obviously using it, i.e. integrating with it), this is a loss of funds for users of the protocol.

Obviously this is an edge case so I marked this as Medium severity.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

Disagree with marking this and the parent issue as Low. The loss of funds here is for those that want to charge the interface fee. The transaction charging the interface fee would fail.

Since we can consider those charging the interface fee as users of the protocol (they are obviously using it, i.e. integrating with it), this is a loss of funds for users of the protocol.

Obviously this is an edge case so I marked this as Medium severity.

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.

KenzoAgada commented 1 year ago

In the main issue I wrote:

Valid finding, but does not break core protocol functionality nor causes loss of user funds. Considering as a valid low.

Hmm. 1) It can be argued that EOAs probably won't issue such transaction in the first place (as the simulating will see it reverts). 2) If smart contracts try to use MultiInvoker with incompatible amount, the transaction will revert. I don't think that can really be considered "loss of user funds". The functionality is somewhat broken. The impact is more like a kind of a DOS. I don't think the impact is high enough for a Sherlock medium.