code-423n4 / 2023-12-initcapital-findings

3 stars 3 forks source link

MoneyMarketHook.sol is not capable of handling and receive WETH in function execute when WETH is unwrapped to ETH #2

Closed c4-bot-3 closed 10 months ago

c4-bot-3 commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-12-initcapital/blob/a53e401529451b208095b3af11862984d0b32177/contracts/hook/MoneyMarketHook.sol#L77

Vulnerability details

Impact

MoneyMarketHook.sol is not capable of handling and receive WETH in function execute when WETH is unwrapped to ETH

Proof of Concept

In the last step of function execute

if (_params.returnNative) {
IWNative(WNATIVE).withdraw(IERC20(WNATIVE).balanceOf(address(this)));
    (bool success,) = payable(msg.sender).call{value: address(this).balance}('');
    _require(success, Errors.CALL_FAILED);
}

the expect flow is the WETH is transfered to MoneyMarketHook.sol

then the WETH is converted to native ETH via

IWNative(WNATIVE).withdraw(IERC20(WNATIVE).balanceOf(address(this)));

then the ETH will be transfered to recipient address

    (bool success,) = payable(msg.sender).call{value: address(this).balance}('');
    _require(success, Errors.CALL_FAILED);

However, in this line of code

IWNative(WNATIVE).withdraw(IERC20(WNATIVE).balanceOf(address(this)));

the MoneyMarketHook.sol has to be able receive the ETH

https://etherscan.io/token/0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2#code#L45

   function withdraw(uint wad) public {
        require(balanceOf[msg.sender] >= wad);
        balanceOf[msg.sender] -= wad;
        msg.sender.transfer(wad);
        Withdrawal(msg.sender, wad);
    }

but because the MoneyMarketHook.sol by default does not implementation payable receiver

the withdraw will revert

Tools Used

Manual Review

Recommended Mitigation Steps

add default payable receiver in MoneyMarketHook.sol to make sure the MoneyMarketHook.sol can receive ETH if the user intends to receive ETH

    // Function to receive Ether. msg.data must be empty
    receive() external payable {}

    // Fallback function is called when msg.data is not empty
    fallback() external payable {}

Assessed type

ETH-Transfer

sashik-eth commented 11 months ago

From my understanding, MoneyMarketHook would be deployed as a proxy that can receive ETH -TransparentUpgradeableProxyReceiveETH, see deploy script: https://github.com/code-423n4/2023-12-initcapital/blob/main/script/DeployBase.sol#L236

hansfriese commented 10 months ago

Looks invalid. Will keep it for the sponsor's confirmation.

c4-judge commented 10 months ago

hansfriese marked the issue as primary issue

c4-sponsor commented 10 months ago

fez-init (sponsor) disputed

fez-init commented 10 months ago

We are using a modified version of transparent proxy that adds the receive functionality in the proxy level.

JeffCX commented 10 months ago

https://github.com/code-423n4/2023-12-initcapital/blob/a53e401529451b208095b3af11862984d0b32177/script/DeployBase.sol#L232

that's right

agree with sponsor

c4-judge commented 10 months ago

hansfriese marked the issue as unsatisfactory: Invalid