Mimo August 2022 contest - peritoflores's results

Lines of code

https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxy.sol#L29-L33 https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxy.sol#L4

Vulnerability details

Impact

PBR proxy owner change protection can bypassed / DoS

PoC

PRBProxy has a protection to prevent malicious delegatecall to overwrite owner.

function execute(address target, bytes calldata data) public payable override returns (bytes memory response) {

...
...

// Check that the owner has not been changed.
if (owner_ != owner) {  @audit   owner change protection
     revert CustomErrors.OWNER_CHANGED(owner_, owner);
  }

...
}

This is an important protection because delegatecall has the option to set any variable in the contract including the owner.

However, you have made some changes to PRB Proxy adding more storage variables (by inheriting Initializable) that are vulnerable to be changed via a malicious call.

abstract contract Initializable {
/**
 * @dev Indicates that the contract has been initialized.
      * @custom:oz-retyped-from bool
      */
    uint8 private _initialized;

/;

that means that _initialized is part of the storage of MIMO proxy.

So the malicious contract called by execute can

1. Overwrite _initalized and set to zero in the delegatecall (owner still is the same so execute will finish correctly).
2. Call initialize() and set a new owner.

Recommended

I believe that it is better to use as little storage variables as possible. Ideally ZERO.

The solution I have been thinking is

1. Remove Initializable contract

2. Remove initialize function

3. Create a constructor to set owner and mark it as immutable.

4. Set minGasReserve as constant (I see no point in this variable, worse that is storage, and I believe that here the vulnerability is from PRB proxy). minGasReserve can be used to DoS if set too high because of an underflow ( uint256 stipend = gasleft() - minGasReserve; )

   Maybe you want to come with a different solution. For example, check that at the end of execute the contract is still initialized.

   However, I have been thinking how to fix this and my proposal solves another bug related to multicall that I will be reporting next.

Lines of code

https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/libraries/BoringBatchable.sol#L36

Vulnerability details

Impact

msg.value in a loop can be used to drain proxy funds

PoC

While BoringBatchable is out of the scope, this bug affects seriously MIMOProxy as it inherits.

Some time ago I read a report about an auditor called samczsung (https://samczsun.com/two-rights-might-make-a-wrong/). I believe that you are having the same problem here.

I will try to explain it as brief as possible but I can add a PoC in QA stage if required.

First step: Draining ETH

This vulnerability comes from the fact that msg.value and msg.sender are persisted in delegatecall.

It is possible to call execute() (which is payable ) from batch() (which is also payable ) because both are public functions. (For now ignore the fact that execute() has access control).

The attacker would call batch() sending, for example, 1 ETH with an array of 100 equal items that call execute()

This execute() will call and external contract 100 times and in every time it will send 1ETH from proxy funds (not from the attacker).

If the receiving contract stores these value then the proxy wallet will be drained.

Second step: Access control bypass scenario

While this is already a high risk and there should be many attacking scenarios I would like to show you a pretty simple one.

Suppose the owner would like to grant access to a target with a normal function (maybe no even payable).

For example suppose that the owner grant access to the function

  function goodFunction() public 

This function has the selector 0x0d092393 . However, for some reason. the owner mistyped the selector and grant access to non existing function 0x0d09392.

Then if the target contract has the so common function.

fallback() external payable {  }

Then the attacker can drain wallet funds using this selector as I explained above.

Recommended

The solution is pretty straightforward.

Remove payable from batch() in BoringBatchable.

it("PoC: should be able to reuse msg.value for multiple deposits", async () => {
const { mimoProxy, vaultActions, vaultsDataProvider, wmatic } = await setup();

//Send ETH to the proxy RECEIVE EXTERNAL PAYABLE
const [owner] = await ethers.getSigners();
owner.sendTransaction({ to: mimoProxy.address, value: DEPOSIT_AMOUNT });
await mimoProxy.execute(vaultActions.address, vaultActions.interface.encodeFunctionData("depositETH"), {
  value: DEPOSIT_AMOUNT,
});
const vaultIdBefore = await vaultsDataProvider.vaultId(wmatic.address, mimoProxy.address);
const vaultBalanceBefore = await vaultsDataProvider.vaultCollateralBalance(vaultIdBefore);
const data = vaultActions.interface.encodeFunctionData("depositETH");
mimoProxy.batch(
  [
    mimoProxy.interface.encodeFunctionData("execute", [vaultActions.address, data]),
    mimoProxy.interface.encodeFunctionData("execute", [vaultActions.address, data]),
  ],
  true,
  { value: DEPOSIT_AMOUNT },
);
const vaultId = await vaultsDataProvider.vaultId(wmatic.address, mimoProxy.address);
const vaultBalanceAfter = await vaultsDataProvider.vaultCollateralBalance(vaultId);
expect(vaultBalanceAfter).to.be.equal(vaultBalanceBefore.add(DEPOSIT_AMOUNT).add(DEPOSIT_AMOUNT));}
);