code-423n4 / 2024-02-wise-lending-findings

11 stars 8 forks source link

Receive functions does not forward funds to master address #246

Closed c4-bot-2 closed 5 months ago

c4-bot-2 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarm/PendlePowerManager.sol#L21-L29 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmControllerBase.sol#L125-L133

Vulnerability details

The receive functions in PendlePowerManager:#21 and PendlePowerFarmControllerBase:#125 are not forwarding the received funds to the master address, simply emitting an ETHReceived event. The transfer to master address is implemented in other contracts (e.g WiseLending:#49).

Impact

It locks ETH inside the affected contracts instead of sending to master address.

Proof of Concept

  1. User sends ETH by mistake to the mentioned contracts
  2. There's no forwarding done so funds likely lost

Tools Used

Manual review

Recommended Mitigation Steps

Implement the transfer so ETH funds don't get locked inside the contracts. Proposed fix:


    receive()
        external
        payable
    {
        _sendValue(
            master,
            msg.value
        );

        emit ETHReceived(
            msg.value,
            msg.sender
        );
    }

## Assessed type

Other
vm06007 commented 5 months ago

This submission is invalid, as it failed to understand code base and see that there is another function for that forwardETH()

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmController.sol#L600-L610

this functions ensure ETH does not get stuck, and also that it does not need to be transferred every time immediately, instead it can be accumulated and sent on DEMAND.

Dismissed.

c4-pre-sort commented 5 months ago

GalloDaSballo marked the issue as insufficient quality report

c4-judge commented 5 months ago

trust1995 marked the issue as unsatisfactory: Invalid

trust1995 commented 5 months ago

@vm06007 What about the first instance? https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarm/PendlePowerManager.sol#L21-L29

c4-judge commented 5 months ago

trust1995 marked the issue as satisfactory

c4-judge commented 5 months ago

trust1995 marked the issue as primary issue

c4-judge commented 5 months ago

trust1995 marked issue #199 as primary and marked this issue as a duplicate of 199

c4-judge commented 5 months ago

trust1995 marked the issue as partial-50

c4-judge commented 5 months ago

trust1995 marked the issue as satisfactory

stalinMacias commented 5 months ago

Hey @trust1995 , I believe this is an invalid report for this reason:

@vm06007 What about the first instance? https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarm/PendlePowerManager.sol#L21-L29

    function _swapStETHintoETH(
        uint256 _swapAmount,
        uint256 _minOutAmount
    )
        internal
        returns (uint256)
    {
       //@audit => The PowerFarm needs to receive the native tokens that Curves will send to it
        return CURVE.exchange(
            {
                fromIndex: 1,
                toIndex: 0,
                exactAmountFrom: _swapAmount,
                minReceiveAmount: _minOutAmount
            }
        );
    }

Looks like the reporter did not understand what is the real purpose of the receive() on the PendlePowerManager.

trust1995 commented 5 months ago

How will the tokens received through _swapStETHintoETH() leave the contract at a future point in time?

stalinMacias commented 5 months ago

How will the tokens received through _swapStETHintoETH() leave the contract at a future point in time?

In the same tx, as part of the closingPosition logic, after swapping stETH for ETH, the execution will either call _closingRouteETH() or _closingRouteToken(), in any of those functions, after repaying the floashloan, the leftover eth is either send to the caller (_closingRouteETH()), or the leftover eth is wrapped and the weth is sent to the caller (_closingRouteToken()).

The swapped native is not left in the power farm, all the swapped eth is used within the same tx.

c4-judge commented 5 months ago

trust1995 marked the issue as unsatisfactory: Invalid