OpenZeppelin / ethernaut

Web3/Solidity based wargame
MIT License
1.98k stars 677 forks source link

Update the DetectionBot of DoubleEntryPoint challenge and the validateInstance of the DoubleEntryPointFactory #745

Open Nfire2103 opened 3 months ago

Nfire2103 commented 3 months ago

I will speak about the DoubleEntryPoint challenge, the one which use Forta.

Currently, this is the solution of this challenge:

    function handleTransaction(address user, bytes calldata msgData) public override {
        // Only the Forta contract can call this method
        require(msg.sender == address(fortaContract), "Unauthorized");
        fortaContract.raiseAlert(user);
        msgData;
    }

With this solution, the method raiseAlert is called all the time which makes revert the delegateTransfer method at each call. In my opinion, it makes no sense to have a method which reverts always.

According to me, the real exploit is to call the delegateTransfer method from the CryptoVault contract because DoubleEntryPoint is an underlying token. It can happend if someone call sweepToken method with LegacyToken address in parameter. The DetectionBot must prevent of this case. So this is my proposition of solution:

    function handleTransaction(address user, bytes calldata msgData) public override {
        // Only the Forta contract can call this method
        require(msg.sender == address(fortaContract), "Unauthorized");

        // Decode the parameters of the delegateTransfer method
        (, , address origSender) = abi.decode(
            msgData[4:],
            (address, uint256, address)
        );

        // The origSender mustn't be the CryptoVault
        // because DoubleEntryPoint is an underlying token,
        // if so raise an alert
        if (origSender == cryptoVaultContract)
            fortaContract.raiseAlert(user);
    }

With this solution, the method raiseAlert is called only when the vulnerability is exploited.

Now, to prevent someone to solve this challenge with a DetectionBot which raises an alert all the time. I also updated the validateInstance method of the DoubleEntryPointFactory. Before trying to sweep token, the validateInstance method will try to emulate a lambda transfer of a user, if the transfer reverts, the validateInstance fails.

I updated unit tests to test my code. I also deployed the new DoubleEntryPointFactory in local environment to test it through ethernaut. Everything is working!

Do I have to push the new build of the DoubleEntryPointFactory contract?

Also, DoubleEntryPoint challenge is the only one which reverts when the validateInstance fails. It doesn't come from of my code, it was already there. Do you want me to fix that? (It's 2 lines)