Bypass access-control check in execute function

[vladbochok]

Description

There is a possibility to bypass the access-control check in execute function. The function itself performs a delegate call to the user-specified address with the specified data. As an access control, the function checks that either it was called by the owner or the owner has previously approved that the sender can call a specified target with specified calldata.

https://github.com/paulrberg/prb-proxy/blob/0cab8248a4c513fa86e4064c352cff054d54ff90/contracts/PRBProxy.sol#L66-L74

The problem is how the selector is calculated. Specifically, calldataload(data.offset) - reads first 4 bytes of data. Imagine data.length == 0, does it mean that calldataload(data.offset) will return bytes4(0)? No.

The solidity function checks that the calldata length is less than needed, but does not check that there is no redundant data in calldata. That means, the function execute(address target, bytes calldata data) will definitely accept data that have target and data, but also in calldata can be other user-provided bytes. As a result, calldataload(data.offset) can read trash, but not the data bytes.

And in the case of executing the function, an attacker can affect the execution by providing trash data at the end of the function. Namely, if the attacker has permission to call the function with some signature, the attacker can call proxy contract bypass check for the signature and make delegate call directly with zero calldata.

Possible Solution

Replace inline assembly

bytes4 selector;
assembly { 
    selector := calldataload(data.offset) 
} 

With:

bytes4 selector = bytes4(data[:4]);

[PaulRBerg]

Hi @vladbochok, thanks very much for creating this issue and reporting your findings.

I'm struggling to understand what is the problem. Yes, it is the case that an envoy can append "trash" data when calling execute, but if the first 4 bytes of that "trash" data wasn't previously approved by the owner, the execution would revert.

It might be worth it to go through an example. Could you share an example for the following cases?

Imagine data.length == 0, does it mean that calldataload(data.offset) will return bytes4(0)? No.

And:

the attacker can call proxy contract bypass check for the signature and make delegate call directly with zero calldata

[PaulRBerg]

Hi @vladbochok, I am planning on closing this issue. Sorry for the hassle - but, if you have a little bit of time, could you please explain the alleged vulnerability again?

As explained in my comment above, I don't see any problem here.

[vladbochok]

I am so sorry, I haven't see your comment and totally forgot about this. There is a explanation on the same issue of fork - https://code4rena.com/reports/2022-08-mimo#h-04-incorrect-implementation-of-access-control-in-mimoproxyexecute.

If that doesn't provide enough information, I can create a test to further illustrate the issue.

[vladbochok]

I'm struggling to understand what is the problem. Yes, it is the case that an envoy can append "trash" data when calling execute, but if the first 4 bytes of that "trash" data wasn't previously approved by the owner, the execution would revert.

Right, so the issue is that an attacker could bypass calling the fallback function if has at least one previously approved function signature.

[PaulRBerg]

Thanks for linking to that code4rena post - I skimmed through it and it's all clear now. This is what made me understand:

The exploit here is if you permitted contract A to run function foo only, A.fallback() is also permitted.

I would classify this as a low-severity bug though, since it's only applicable after the user granted permission to a contract, and it's also applicable only if that contract has a fallback function that can do funky things. Presumably a user wouldn't grant permission to any target contract at random.

However, this is important, and must be fixed. Thanks a lot for taking the time to report this, @vladbochok!

[PaulRBerg]

I just found the time to write a test that proves the existence of this bug:

function test_RevertWhen_TargetHasFallbackFunction() external {
    proxy.setPermission({
        envoy: users.envoy,
        target: address(targets.dummyWithFallback),
        selector: targets.dummyWithFallback.foo.selector,
        permission: true
    });
    changePrank(users.envoy);
    bytes memory usualCalldata = abi.encodeWithSelector(
        proxy.execute.selector,
        address(targets.dummyWithFallback),
        new bytes(0)
    );
    bytes memory data = abi.encodePacked(usualCalldata, targets.dummyWithFallback.foo.selector);
    (bool success, ) = address(proxy).call(data);
    assertFalse(success);
}

The assertFalse assertion will fail, because the call is successful, and the fallback function is called on the target.

Will fix this in a commit later today.

[PaulRBerg]

Fixed in 7594489.