hats-finance / Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573

0 stars 1 forks source link

Arbitrum rewards in PendlePowerFarms will be lost permanently, when claimed. #23

Open hats-bug-reporter[bot] opened 6 months ago

hats-bug-reporter[bot] commented 6 months ago

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

Description: Description\ Arbitrum rewards in PendlePowerFarms will be lost permanently, when claimed.

The claimArb function in PendlePowerFarmController allows for claiming Arbitrum rewards. The rewards accrued can with withdrawn to the master account as in the code below.

PendlePowerFarmController::claimArb() function

    function claimArb(
        uint256 _accrued,
        bytes32[] calldata _proof
    )
        external
        onlyArbitrum
    {
        ARB_REWARDS.claim(
            master,
            _accrued,
            _proof
        );

        emit ClaimArb(
            _accrued,
            _proof,
            block.timestamp
        );
    }

Problem: renounceOwnership() 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.

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, the accrued rewards on Arbitrum chain can be deemed lost. The reason being, the accured reward tokens on claiming are routed to master account which was set to zero address when renounced.

So, the rewards are pemanently lost.

Attack Scenario\ a) Let the protocol accumulate rewards on artibutrm chain via PendlePowerFarms.

b) Call claimArb() function, and the accurred rewards will be deposited in master account.

c) now, call renounceOwnership() function on PendlePowerFarmController(). After this call, the master state variable in PendlePowerFarmController() storage layout should be zero address.

c) Now, call claimArb() on the PendlePowerFarmController contract. This time the rewards will not reach the earlier nominated master account. Instead, the rewards 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

POC3.txt

  1. Revised Code File (Optional)

Recommendation is to create a dedicated account for collecting the rewards tokens in the PendlePowerFarmController contract. revise the claimArb() function to route the tokens to the new dedicated address.

Add a new state variable as arbRewardReceipientAddress. Assign a valid address in the contructor.

PendlePowerFarmController::claimArb() function 
function claimArb(
    uint256 _accrued,
    bytes32[] calldata _proof
)
    external
    onlyArbitrum
{
    ARB_REWARDS.claim(
-        master,
+        arbRewardReceipientAddress, 
        _accrued,
        _proof
    );

    emit ClaimArb(
        _accrued,
        _proof,
        block.timestamp
    );
}


**Files:**
  - Poc4.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmRpbLDDzWgiRxcK9b2vwh23fxmLHHpeDsQGL9LiHpmtRs)
vm06007 commented 6 months ago

Hello @betharavikiran, again you are making a mistake here with your claim that something WILL be lost, and with wrong assumption that protocol controller would be calling renounceOwnership() in order to make things "decentralized"

This is not the case, to understand better please refer to an answer already given in here: https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/issues/20

vm06007 commented 6 months ago

With that said I would nearly call this low or informational submission on topic of decentralization which is not even in the scope to start with. This is topic of deciding to have renounceOwnership() or not, which is a topic of design choice and level of control/decentralization when it comes to final stage (or decision to migrate abandon old protocol and move to new one) read here more: https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/issues/20