Submission Details
Severity:
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 about 1 year ago
Submission Judgement Published Reason: Known issue
Assigned finding tags:
finding-lack-contract-existence
Invalid [known issue [Low-15]](https://github.com/Cyfrin/2024-07-biconomy/issues/1)
Prize pool breakdown
Total prize
30,000 USDC
nSLOC
1,43321 USDC / LOC
25,000 USDC
2,750 USDC
2,250 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.