code-423n4 / 2023-01-numoen-findings

0 stars 0 forks source link

Lock of value at `Payment` if value added to the call #273

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-numoen/blob/c75b39dc93fe327039b097ba149e1abe90d3b2f5/src/periphery/Payment.sol#L25-L46

Vulnerability details

Lock of value at Payment if value added to the call

Summary

If Payment balance of weth is still 0, ether can be lost calling a payable function with ether.

There are 2 contracts that inherit from Payment:

Vulnerability Detail

unwrapWETH can lock ether added to the call of unwrapWETH() as it will not revert as it bypasses

First someone call unwrapWETH or sweepToken when balanceWETH for Payment is 0, that would result on nothing reverting and no value of the call being used.

  function unwrapWETH(uint256 amountMinimum, address recipient) public payable {
    uint256 balanceWETH = Balance.balance(weth);
    if (balanceWETH < amountMinimum) revert InsufficientOutputError();

    if (balanceWETH > 0) {
      IWETH9(weth).withdraw(balanceWETH);
      SafeTransferLib.safeTransferETH(recipient, balanceWETH);
    }
  }

Then refundETH can be called to recover the ETH locked in the contract, however, it can be front runned and ETH be stolen

  function refundETH() external payable {
    if (address(this).balance > 0) SafeTransferLib.safeTransferETH(msg.sender, address(this).balance);
  }

Impact

Lose of assets

Code Snippet

https://github.com/code-423n4/2023-01-numoen/blob/c75b39dc93fe327039b097ba149e1abe90d3b2f5/src/periphery/Payment.sol#L25-L46

Tool used

Manual analysis

Recommendation

Remove payable where value can not be used or create a condition for when msg.value is not 0

berndartmueller commented 1 year ago

Downgrading to QA (Low) as this represents a user error.

c4-judge commented 1 year ago

berndartmueller changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

berndartmueller marked the issue as grade-c