Why this delegatecall attack not working?

Question

I am confused by this simple contract. The main purpose is to bypass the OnlyContract() modifier.

Victim contract.

pragma solidity 0.7.6;

contract Victim {

    modifier OnlyContract() {
        require(msg.sender == address(this), "You are not this contract");
        _;
    }

    function withdraw(uint256 amount) external {
        (bool success,) = msg.sender.call{value : amount}("");
        require(success, "Withdraw failed");
    }

    function ContractWithdraw(address payable to_, uint256 value) external OnlyContract {
        require(address(this).balance >= value, "No funds");
        (bool success,) = to_.call{value : value}("");
        require(success, "Contract Withdraw failed");
    }
}

Malicious Contract

pragma solidity 0.7.6;

contract Malicious {
    address payable owner;

    constructor() public {
        owner = msg.sender;
    }
    
    receive() external payable {
        address x = address(0x0000000000);
        (bool success, bytes memory data) = x.delegatecall(abi.encodeWithSignature("ContractWithdraw(address,uint256)", address(this), 1000000000000000000));
    }
    
    function Attack() public {
        x.withdraw();
    }
    
    function withdraw_all() public {
        owner.transfer(address(this).balance);
    }
}

The flow is like this:

1. Attacker call Attack() on Malicious contract.
2. Victim contract send ETH to Malicious contract.
3. Victim contract go into Malicious fallback contract and delegatecall to his own contract.
4. From here, msg.sender delegatecall has changed to Victim contract.

But in reality, the delegate call transaction reverted. I think I've bypassed the OnlyContract() modifier.

Why did the attack fail?

Answer 1

Your OnlyContract() modifier checks the msg.sender and requires address(this).

When Malicious delegatecalls, the context is Malicious which is to say that OnlyContract will not be fooled - it will see the foreign address, not its own.

Contract A ----- delegates ----> Contract B ---- calls ----> Contract C ---> msg.sender = A


function delegateToB() ...

                                 function doSomethingToC() public view returns(address) {
                                    return C(address).doSomething();



Contract C 

function doSomething() public view returns(address) {
  return msg.sender;
}

When A delegates to B, you can conceptualize it as A has imported bytecode from B but it continues to execute in the context of A - A's storage, A is msg.sender, address(this) is A (even if the code came from B). So ... when C is called upon to report who, exactly, has called it, it reports A. Your modifier disapproves and it defends the victim.

Hope it helps.