Eth sent directly to WiseLending contract will be lost

[hats-bug-reporter[bot]]

Github username: @https://github.com/betharavikiran Twitter username: @ravikiranweb3 Submission hash (on-chain): 0xe32d02a005dd5cb6f2bd97508b75f8981400a43137f25c1447520c1275ca5393 Severity: high

Description: Description\ Eth sent directly to WiseLending contract will be lost

The Eth sent directly to WiseLending contract will be accepted via the receive() function which is a low level call, which routes those funds to the master account.

WiseLending::receive() function

receive()
        external
        payable
    {
        if (msg.sender == WETH_ADDRESS) {
            return;
        }

        _sendValue(
            master,
            msg.value
        );
    }

The Wiselending protocol has a design intention to be totally decentralized when the circumstances permit and to provision that final state, the protocol has provided for renounceOwnership() after which the protocol will be fully decentralized.

Vulnerability

When the renounceOwnership() will be executed, the master account will be set to zero address account as in the below code snippet.

So, then the protocol decides to renounce ownership, the master account will be set to zero. After that point onwards, when funds are sent to WiseLending contract directly will be sent to zero address and hence permanently lost.

OwnableMaster::renounceOwnership()

function renounceOwnership()
        external
        onlyMaster
    {
        master = ZERO_ADDRESS;
        proposedMaster = ZERO_ADDRESS;

        emit RenouncedOwnership(
            msg.sender
        );
    }

Attack Scenario\ a) Send funds directly to WiseLending using low level call with data parameter as empty, the funds will be routed to master account via WiseLending Contract. The balance should show that number.

b) After this, call renounceOwnership() function on WiseLending(). After this call, the master state variable in WiseLending() storage layout should be zero address. None of the functions attached with onlyMaster() modifier should work. They all should revert with "NotMaster" error.

c) Now, again send funds directly to WiseLending using the low level call with data parameter as empty. This time the funds will not reach the earlier nominated master account. Instead, the funds flows into zero account and are lost permamently as no one has access to the private keys of zero address account.

Attachments

1. Proof of Concept (PoC) File

POC1.txt

2. Revised Code File (Optional)

Recommendation is to revert if master address is zero address. Below is an approach to prevent loosing tokens,.

contract WiseLending is PoolManager {

receive()
    external
    payable
{
    if (msg.sender == WETH_ADDRESS) {
        return;
    }

+   require(master!=address(0x0),"WiseLending cannot accept Eth funds");    

    _sendValue(
        master,
        msg.value
    );
}

Files:

• POC1.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmZd1TyTzpoPE93BeuLF1XcnsDfTgdxrh7iP7fmqGhHNRu)

[vm06007]

Hello @betharavikiran, I think this is nearly trying to squeeze something out of the competition. First of all you've stated remarkably wrong assumption when saying when the renounceOwnership() will be executed, assuming this is the final stage in order for protocol to be decentralized. In fact this is wrong. Let me educate you on why this is not the case.

Common patter of masterController does have ability for master to call renounceOwnership(), only in case it is demanded in by certain conditions, but not to achieve decentralization. There are other ways to achieve this. In fact the plan would be to call proposeNewMaster() and overwrite ownership to a DAO or a smart-contract that can only forward ETH to the DAO or other beneficiary account. (without any other functions intended in such ruling contract).

Another approach is to set master in control to be time-lock contract that can only perform some operations but with a time-delay on execution. What we are discussing here is a design choice to have renounceOwnership function or not. We would rather have it in case it is demanded that ownership is completely destroyed for any reason (for example when abandoning old protocol and migrating to a new one)

However final intention for protocol to achieve decentralization is to overwrite multi-sig master to a controller contract that for example can only forward ETH or do pre-defined actions without actual master controlling this contract in charge.

With that being said I would nearly mark this as low or informational item with consideration to remove renounceOwnership() which we most likely will not do anyway and as planned to achieve decentralization by putting a time-lock contract in charge without ability to change other parameters and only forward ETH or do pre-defined actions without master in charge.

Also the title is misleading, if ETH sent directly to WiseLending they are not lost from the get-go. Only if master is renounced, but the reasoning of why this is not the case to even happen described above. Maybe if you've mark this as low and put "might be lost" instead of "will be lost" in the title with a bit more understanding on how to achieve decentralization we would give you some credit.

[betharavikiran]

I think a basic respect for security researcher is missing here. The discussion is more about money and attempt to squeezing it rather than looking at the issue identification and looking at it subjectively as a problem that needs a fix.

If you disagree with my reporting, Call renounce ownership on the contract after going live and you will learn what the consequences will be. That is what I reported. It's that simple. Does the contract allow doing, if called, will the protocol face the consequences.

The code describes the situation/circumstances and issue and not what you think as solutions for that not happening, keeping that in mind

Call the renounce ownership on live day and tell me none of the problems I reported will happen.

I understand your concerns about paying and I am not looking for that. I know there are protocols out there who respect the efforts of security researcher more from issue point of view.

I committed myself to look into this protocol until 19th, but I realised my mistake for the efforts I put in. This protocol is more worried about the bounty amount than Vulnerabilities.

I can contribute to some other protocol who can respect my efforts and are able to look at vulnerability subjectively, not in terms of how much they have to pay. I really understand you pain point.

Thanks.

On Sun, 11 Feb, 2024, 6:11 pm Vitalik Marincenko, @.***> wrote:

Hello @betharavikiran https://github.com/betharavikiran, I think this is nearly trying to squeeze something out of the competition. First of all you've stated remarkably wrong assumption when saying when the renounceOwnership() will be executed, assuming this is the final stage in order for protocol to be decentralized. In fact this is wrong. Let me educate you on why this is not the case.

Common patter of masterController does have ability for master to call renounceOwnership(), only in case it is demanded in by certain conditions, but not to achieve decentralization. There are other ways to achieve this. In fact the plan would be to call proposeNewMaster() and overwrite ownership to a DAO or a smart-contract that can only forward ETH to the DAO or other beneficiary account. (without any other functions intended in such ruling contract).

Another approach is to set master in control to be time-lock contract that can only perform some operations but with a time-delay on execution. What we are discussing here is a design choice to have renounceOwnership function or not. We would rather have it in case it is demanded that ownership is completely destroyed for any reason (for example when abandoning old protocol and migrating to a new one)

However final intention for protocol to achieve decentralization is to overwrite multi-sig master to a controller contract that for example can only forward ETH or do pre-defined actions without actual master controlling this contract in charge.

With that being said I would nearly mark this as low or informational item with consideration to remove renounceOwnership() which we most likely will not do anyway and as planned to achieve decentralization by putting a time-lock contract in charge without ability to change other parameters and only forward ETH or do pre-defined actions without master in charge.

Also the title is misleading, if ETH sent directly to WiseLending they are not lost from the get-go. Only if master is renounced, but the reasoning of why this is not the case to even happen described above. Maybe if you've mark this as low and put "might be lost" instead of "will be lost" in the title with a bit more understanding on how to achieve decentralization we would give you some credit.

— Reply to this email directly, view it on GitHub https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/issues/20#issuecomment-1937701627, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACTGZVKWOBRG45F344D5GQDYTC37DAVCNFSM6AAAAABDDPMDEKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZXG4YDCNRSG4 . You are receiving this because you were mentioned.Message ID: <hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/issues/20/1937701627 @github.com>

[vm06007]

@betharavikiran seems you've been missing the point of how and what you've reported.

Based on that I would say its also arguable who is a researcher and what is the motivation here. What we want is a fair competition with on point reports. Not a manipulated opinions that do not represent reality when you are claiming something WILL absolutely happen with false assumptions that certain function WILL be called for unjustified reasons. Here you are in the wrong (I've already explained why initial assumption is wrong and your whole submission is only based on arguing about that Wise Lending will absolutely call that renounceOwnership function to make it decentralized). Also it seems you are marking everything as "high" where in reality (as stated we want fair and actual vulnerabilities found) it is nearly a low or "informational" findings.

I also believe respect is earned rather than just blindly accounted with self-proclaimed titles. It is hard to earn it when you just spam same thing marking it as "high" without even understanding that "decentralization" is achieved not through setting master to zero address, or you could take a peek into scope where I'm sure it was mentioned decentralization topic is not in the scope.

Take UniswapV2 for example. There is also a function setFeeToSetter() which current feeSetter can change to zeroAddress renouncing any ability to collect any fees in future if feeTo is also never updated or set to 0xdeaD address. it is possible by design and its a question of the design of the protocol, but it is done for a reason to have that option and not an exploit, but aligned behavior to have that ability in case of a migration.

You could state your report more closer to reality reporting that if renouceOwnership() is called, then fees or ETH will also follow same address which I think is expected behavior. But it is not nice to manipulate an opinion that this expected behavior for the protocol to execute this function in order to achieve decentralization. It's a design choice to have that function or not to have it, or to make a different role for fee address destination perhaps, which again is a suggestion or low at maximum, but not "high" as something inevitable with a backstory how protocol will absolutely going to call that renounce function. Context matters here understand that decentralization is achieved in our scenario completely differently and does not even need to rely on presence of that function in the contract.

Here is also why we would keep that function: Since we are using 2-step ownership change, proposeOwner => claimOwner, there is no way to set it to 0x0 address and just renounce because 0x0 address cannot claim ownership. So for that reason this function is added as common pattern whenever OwnableMaster.sol is used. You could also open one report item instead of 5 different items marking them as "high" reporting same design choice for OwnableMaster.sol to have that renounce pattern. Which again is intentional for specific scenario, but absolutely not a "high" because it does not block road to decentralization and does not sacrifice ability to collect fees if multi-sig would like to step-down from being in control.

[vm06007]

@betharavikiran you are saying something like: Call the renounce ownership on live day and tell me none of the problems I reported will happen. If we want to call it we will call it, but it will not create a problem since it is expected behavior meaning nobody collect ETH that is being sent directly to the contract. When ETH is being sent directly to the contract I can tell you it is a moral question of address in control to decide between a) - return that ETH and b) keep that ETH. If this ability does not present to rescue ETH then there is no question to decide what to do, it simply follow the pattern that any funds sent to contract are not redeemable. (as expected if renounceOwnership() is called)

However with that phrase you are still suggesting that we are planning to call this function on day we go live to make it "decentralized" which is again not the case if we would want to remove ownership control and move from multi-sig then we would change ownership to DAO or put a blind/simple contract as master that only does ETH forwarding or some pre-defined actions without ability to update master address.

If you take critics to your submission this closely perhaps indeed you don't need to spam your findings. We would appreciate more truthful submissions that does reflect its severity and take in account what is in the scope.

What I've posted is also not a solution, keep that in mind, it is exact approach that protocol would take to make it decentralized since master has more roles than just getting accidentally sent ETH into contract directly. Suggesting this is an issue is silly and expect to renounce ownership. Because you've failed to recognize master is also responsible for calling other vital functions such as "CreatePool" for example. If we would renounce ownership as you are suggesting should happen to achieve decentralization, then directly sent ETH to the contract would be least of our concerns. Master has more things to do, so it would be obviously set as DAO contract or time-lock contract that is capable to forward ETH its not a solution its a plan. Keep in mind CreatingPools is not the only responsibility there's more.