hats-finance / Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573

0 stars 1 forks source link

Excess tokens above LP assets in PendlePowerFarms will be lost permanently. #22

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

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

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

Description: Description\ Excess tokens above LP assets in PendlePowerFarms will be lost permanently.

The skim function in PendlePowerFarmController allows the protocol owner account to withdraw access tokens above LP accesss to master account. Refer to the below skim() function which computes the difference between balance for the pendleMarket and total assets in the farm.

Any access assets are transferred to master account.

PendlePowerFarmController::skim() function

    function skim(
        address _pendleMarket
    )
        external
        returns (uint256)
    {
        address childMarket = pendleChildAddress[
            _pendleMarket
        ];

        if (childMarket == ZERO_ADDRESS) {
            revert WrongAddress();
        }

        uint256 balance = IPendleMarket(_pendleMarket).balanceOf(
            address(this)
        );

        uint256 totalAssets = IPendlePowerFarmToken(
            childMarket
        ).totalLpAssets();

        if (balance < totalAssets + 1) {
            revert NothingToSkim();
        }

        uint256 difference = balance
            - totalAssets
            + 1;

        _safeTransfer(
            _pendleMarket,
            master,
            difference
        );

        return difference;
    }

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, when skim function is called for every pendleMarket will be routed to master account which was set to zero and hence all tokens withdrawn via skim function will be lost permanently.

Attack Scenario\ a) Let the protocol accumulate funds via PendleFarms so that there is a balance in one of the pendle markets.

b) Call skim first to check if the funds are received in master account.

c) Let the protocol accumulate funds again so that there is a balance in one of the pendle Markets above the totalAssets.

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

c) Now, call skim on the PendlePowerFarmController contract. 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

POC3.txt

  1. Revised Code File (Optional)

Recommendation is to create a dedicated account for collecting the additional tokens in the PendlePowerFarmController contract. Review the skim function to route the tokens to the new dedicated address.

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

PendlePowerFarmController::skim() function 
function skim(
    address _pendleMarket
)
    external
    returns (uint256)
{
    address childMarket = pendleChildAddress[
        _pendleMarket
    ];

    if (childMarket == ZERO_ADDRESS) {
        revert WrongAddress();
    }

    uint256 balance = IPendleMarket(_pendleMarket).balanceOf(
        address(this)
    );

    uint256 totalAssets = IPendlePowerFarmToken(
        childMarket
    ).totalLpAssets();

    if (balance < totalAssets + 1) {
        revert NothingToSkim();
    }

    uint256 difference = balance
        - totalAssets
        + 1;

    _safeTransfer(
        _pendleMarket,
    -    master,
    +    protocolReceipientAddress,
        difference
    );

    return difference;
}


**Files:**
  - Poc3.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmW4HsNC5pkqsJr2P1Kr8atLk6ftXKitxZXjxpTSdAi7Hu)
vm06007 commented 7 months ago

Hello @ravikiranweb3 I think again this is out of context for topic of decentralization and wrong assumption how master address is used. Please refer to my comments here: https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/issues/20 and here: https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/issues/23

Your assumption here: 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. - is wrong (please read in #20 and #23 why it is wrong) + topic of decentralization is out of scope for this competition.

At best this is low or informational, but not high as falsely stated.