HardhatFoundry
30,000 USDC
View results
Submission Details
Severity: low
Invalid

Lack of contract existence check on delegatecall in `ExecutionHelper::_handleDelegateCallExecutionAndReturnData`, `ExecutionHelper::_handleDelegateCallExecution`will result in unexpected behavior

Description:

ExecutionHelper contract uses the delegatecall. If the delegate contract is incorrectly set or is self-destructed, ExecutionHelper may not be able to detect failed executions. ExecutionHelper::_handleDelegateCallExecution and ExecutionHelper::_handleDelegateCallExecutionAndReturnData These function lacks a contract existence check, A delegatecall to a destructed contract will return success. Due to the lack of
contract existence checks, transactions may appear to be successful
even if they fail.

Imapct:

Failed transactions will appear to be successful as they won't revert

Proof Of Concept:

Set Up an empty MockDelgate Contract

//SPDX-License-Identifier:MIT
pragma solidity ^0.8.25;
contract MockDelegateTargetEmpty{}

These test functions doesn't revert

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.26;
import "../../../shared/TestAccountExecution_Base.t.sol";
import {MockDelegateTargetEmpty} from "contracts/mocks/MockDelgateTargetEmpty.sol";
/// @title TestAccountExecution_TryExecuteSingle
/// @notice This contract tests single execution attempts using the try method in the account execution system.
contract TestAccountExecution_TryExecuteSingle is TestAccountExecution_Base {
MockDelegateTarget delegateTarget;
MockDelegateTargetEmpty delegateTargetEmpty;
/// @notice Sets up the testing environment.
function setUp() public {
setUpTestAccountExecution_Base();
delegateTarget = new MockDelegateTarget();
delegateTargetEmpty = new MockDelegateTargetEmpty();
}
/// @notice Tests successful execution of a single operation.
function test_ExecuteDelegateCall_Success() public {
(bool res, ) = payable(address(BOB_ACCOUNT)).call{ value: 2 ether}(""); // Fund BOB_ACCOUNT
assertEq(res, true, "Funding BOB_ACCOUNT should succeed");
// Initial state assertion
assertEq(counter.getNumber(), 0, "Counter should start at 0");
// Create calldata for the account to execute
// address valueTarget = makeAddr("valueTarget");
// uint256 value = 1 ether;
bytes memory sendValue =
abi.encodeWithSelector("");
//abi.encodeWithSelector(MockDelegateTarget.sendValue.selector, valueTarget, value);
// placeholder
Execution[] memory execution = new Execution[](1);
execution[0] = Execution(address(counter), 0, abi.encodeWithSelector(Counter.incrementNumber.selector));
// Build UserOperation for single execution
PackedUserOperation[] memory userOps = buildPackedUserOperation(BOB, BOB_ACCOUNT, EXECTYPE_DEFAULT, execution, address(VALIDATOR_MODULE));
bytes memory userOpCalldata = abi.encodeCall(
Nexus.execute,
(
ModeLib.encode(
CALLTYPE_DELEGATECALL, EXECTYPE_DEFAULT, MODE_DEFAULT, ModePayload.wrap(0x00)
),
abi.encodePacked(address(delegateTargetEmpty), sendValue)
)
);
userOps[0].callData = userOpCalldata;
// Sign the operation
bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOps[0]);
userOps[0].signature = signMessage(BOB, userOpHash);
ENTRYPOINT.handleOps(userOps, payable(address(BOB.addr)));
// Assert that the value was set ie that execution was successful
// assertTrue(valueTarget.balance == value);
}
/// @notice Tests successful execution of a single operation.
function test_TryExecuteDelegateCall_Success() public {
(bool res, ) = payable(address(BOB_ACCOUNT)).call{ value: 2 ether}(""); // Fund BOB_ACCOUNT
assertEq(res, true, "Funding BOB_ACCOUNT should succeed");
// Initial state assertion
assertEq(counter.getNumber(), 0, "Counter should start at 0");
// Create calldata for the account to execute
// address valueTarget = makeAddr("valueTarget");
// uint256 value = 1 ether;
bytes memory sendValue =
abi.encodeWithSelector("");
//abi.encodeWithSelector(MockDelegateTarget.sendValue.selector, valueTarget, value);
// placeholder
Execution[] memory execution = new Execution[](1);
execution[0] = Execution(address(counter), 0, abi.encodeWithSelector(Counter.incrementNumber.selector));
// Build UserOperation for single execution
PackedUserOperation[] memory userOps = buildPackedUserOperation(BOB, BOB_ACCOUNT, EXECTYPE_TRY, execution, address(VALIDATOR_MODULE));
bytes memory userOpCalldata = abi.encodeCall(
Nexus.execute,
(
ModeLib.encode(
CALLTYPE_DELEGATECALL, EXECTYPE_TRY, MODE_DEFAULT, ModePayload.wrap(0x00)
),
abi.encodePacked(address(delegateTargetEmpty), sendValue)
)
);
userOps[0].callData = userOpCalldata;
// Sign the operation
bytes32 userOpHash = ENTRYPOINT.getUserOpHash(userOps[0]);
userOps[0].signature = signMessage(BOB, userOpHash);
ENTRYPOINT.handleOps(userOps, payable(address(BOB.addr)));
// Assert that the value was set ie that execution was successful
// assertTrue(valueTarget.balance == (value));
}
}

Recommeded Mitigation:

Short term, implement a contract existence check before each delegatecall.

function _handleDelegateCallExecutionAndReturnData(bytes calldata executionCalldata, ExecType execType) internal returns(bytes[] memory returnData) {
(address delegate, bytes calldata callData) = executionCalldata.decodeDelegateCall();
returnData = new bytes[](1);
bool success;
+ bool res;
+ assembly {
+ res := gt(extcodesize(newImplementation), 0)
+ }
+ require(res, InvalidImplementationAddress());
if (execType == EXECTYPE_DEFAULT) {
returnData[0] = _executeDelegatecall(delegate, callData);
}
else if (execType == EXECTYPE_TRY) {
(success, returnData[0]) = _tryExecuteDelegatecall(delegate, callData);
if (!success) emit TryDelegateCallUnsuccessful(0, returnData[0]);
}
else revert UnsupportedExecType(execType);
}
function _handleDelegateCallExecution(bytes calldata executionCalldata, ExecType execType) internal {
(address delegate, bytes calldata callData) = executionCalldata.decodeDelegateCall();
+ bool res;
+ assembly {
+ res := gt(extcodesize(newImplementation), 0)
+ }
+ require(res, InvalidImplementationAddress());
if (execType == EXECTYPE_DEFAULT) _executeDelegatecall(delegate, callData);
else if (execType == EXECTYPE_TRY) _tryExecuteDelegatecall(delegate, callData);
else revert UnsupportedExecType(execType);
}
Updates

Lead Judging Commences

0xnevi Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

finding-lack-contract-existence

Invalid [known issue [Low-15]](https://github.com/Cyfrin/2024-07-biconomy/issues/1)

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.