Vagner - _unwrap in MultiInvoker.sol can revert every time in some cases which will make the users not being able to _liquidate or _withdraw with warp to true

[sherlock-admin]

Vagner

high

_unwrap in MultiInvoker.sol can revert every time in some cases which will make the users not being able to _liquidate or _withdraw with warp to true

Summary

_withdraw makes some wrong assumptions which could lead to reverts all the time if DSU protocol will be in debt.

Vulnerability Detail

The function _withdraw is called in _liquidate, _vaultUpdate and _update, and if the wrap is set to true, which will be all the time in the case of _liquidate, it will try to call _unwrap https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L262
After that _unwrap will check if the address of batcher is 0, which can be the case if it is not set up in the constructor, or if the balanceOf USDC of batcher is less than the amount intended to withdraw
https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L287
and if any of that will be true it will try to call the redeem function on reserve with the intended amount
https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L288
and then transfers the amount to the receiver
https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L289
The problem relies in the fact that the protocol assumes that the amount that is used in redeem function will always be equal to the amount of USDC returned, which is not the case.
As can be seen in the code of the reserve here the redeemAmount is a calculation that depends on the redeemPrice which depends on the debt of the protocol
https://github.com/emptysetsquad/emptyset/blob/c5d876fbd8ff1fac988898b77ef5461971f9fdd2/protocol/contracts/src/reserve/ReserveComptroller.sol#L125
The whole calculation of redeemPrice is done depending of the reserveRatio of the Protocol
https://github.com/emptysetsquad/emptyset/blob/c5d876fbd8ff1fac988898b77ef5461971f9fdd2/protocol/contracts/src/reserve/ReserveComptroller.sol#L81-L84
so in the case where DSU will make some bad decisions and it will be in debt the redeemAmount calculated will be less than the actual amount inserted into redeem function, and that redeemAmount will be the one actual transferred to the MultiInvoker.sol
https://github.com/emptysetsquad/emptyset/blob/c5d876fbd8ff1fac988898b77ef5461971f9fdd2/protocol/contracts/src/reserve/ReserveComptroller.sol#L130
So when _unwrap will try to transfers the same amount of USDC
https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L289
the transaction would revert because the contract would not have enough USDC in the contract, making _vaultUpdate, _update with warp to true on _withdraw reverting all the time and _liquidate reverting 100% of the time. Since the protocol stated that they want to be aware of any issue that might arise with the integration of DSU

I decided to specify this issue which could happen can cause a lot of damage to the users.

Impact

Impact is a high one since it will cause the liquidation process to fail and also withdrawing funds to fail.

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L285-L289

Tool used

Manual Review

Recommendation

Since the contract doesn't store any USDC, you could transfer the whole balance of the contract every time after you call redeem or do balance before and balance after, and transfer the difference. It is important to take special care when you implement other protocols since any failures will also break your protocol.

[sherlock-admin]

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

141345 commented:

l

n33k commented:

low

panprog commented:

medium because the situation is very unlikely

[VagnerAndrei26]

Escalate :
I believe this issue to be valid and at least a valid medium under the considerations of Sherlock rules which states

since the external protocol the Perennial team is working, DSU, is not controlled by the them and since they specified in the ReadMe that they want to know any issues that might arise because of DSU integration,

which I provided in the report. The way the code is written right now might arise problems in case where DSU fails/ becomes insolvent and it can be solved pretty easily in the solution I provided, so considering all of this, this issue should be evaluated at least as a Medium since the impact is a high one, by blocking the whole liquidation process and withdrawing with wrap, and probability of happening can be low.

[sherlock-admin2]

Escalate :
I believe this issue to be valid and at least a valid medium under the considerations of Sherlock rules which states

since the external protocol the Perennial team is working, DSU, is not controlled by the them and since they specified in the ReadMe that they want to know any issues that might arise because of DSU integration,

which I provided in the report. The way the code is written right now might arise problems in case where DSU fails/ becomes insolvent and it can be solved pretty easily in the solution I provided, so considering all of this, this issue should be evaluated at least as a Medium since the impact is a high one, by blocking the whole liquidation process and withdrawing with wrap, and probability of happening can be low.

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.

[141345]

The condition is

if it is not set up in the constructor, or if the balanceOf USDC of batcher is less than the amount intended to withdraw

But at first place, USDC is wrapped into DSU by batcher.

File: perennial-v2\packages\perennial-extensions\contracts\MultiInvoker.sol
271:     function _wrap(address receiver, UFixed18 amount) internal {

277:             // Wrap the USDC into DSU and return to the receiver
278:             batcher.wrap(amount, receiver);

So the USDC in batcher is expected to be fully backed.

Hence low/info severity is more appropriate.

[VagnerAndrei26]

The condition is

if it is not set up in the constructor, or if the balanceOf USDC of batcher is less than the amount intended to withdraw

But at first place, USDC is wrapped into DSU by batcher.

File: perennial-v2\packages\perennial-extensions\contracts\MultiInvoker.sol
271:     function _wrap(address receiver, UFixed18 amount) internal {

277:             // Wrap the USDC into DSU and return to the receiver
278:             batcher.wrap(amount, receiver);

So the USDC in batcher is expected to be fully backed.

Hence low/info severity is more appropriate.

Yes that is true, but that is not the issue that I'm talking about here, I'm not saying that there will not be enough funds in the batcher to be paid back. The issue that I've talked here is the fact that DSU protocol can borrow DSU, which will increase the debt of the protocol, as can be seen here
https://github.com/emptysetsquad/emptyset/blob/c5d876fbd8ff1fac988898b77ef5461971f9fdd2/protocol/contracts/src/reserve/ReserveComptroller.sol#L143-L150
in that case, when the redeem will be called and the redeem amount is calculated redeemPrice which is used in the calculation can be less than one
https://github.com/emptysetsquad/emptyset/blob/c5d876fbd8ff1fac988898b77ef5461971f9fdd2/protocol/contracts/src/reserve/ReserveComptroller.sol#L93-L95
, since the DSU protocol is in debt and redeeming will not be done 1-1, less USDC will be transferred to the MultiInvoker.sol than the amount specified in redeem. Because of that trying to push exactly the specific amount used in redeem, will fail and revert, because the contract will not have enough funds. That's why I specified that transferring the whole balanceOf(address(this)) would solve this issue of blocking liquidation and withdrawing, since the contract doesn't hold any funds and just act as a middle man. So this is a risk associated with the DSU integration that can mess important functionalities of Perennial and also block funds.

[141345]

Sorry I misunderstood the report.

The DSU reserve has borrow function result in debt, and the redeem amount will not be enough. Or we can say it is when DSU depeg from USDC.

Seems an edge case issue.

[VagnerAndrei26]

Sorry I misunderstood the report.

The DSU reserve has borrow function result in debt, and the redeem amount will not be enough. Or we can say it is when DSU depeg from USDC.

Seems an edge case issue.

Yep, it is an edge case, that's why in my escalation I specify that should be treated at least as a medium, since the damage exist and can be quite high and as for probability, it depends a lot on how DSU maintain their balance sheets, since the function can be called anytime by the owners, and since it was specify that Perennial team wants to know integration issue that could occur with DSU, it is in their advantage to protect their protocol against this case.

[hrishibhat]

@arjun-io

[hrishibhat]

Result:
Medium
Unique
Considering this issue a valid medium given the edge case

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• VagnerAndrei26: accepted

[jacksanford1]

From WatchPug:

Acknowledged