code-423n4 / 2022-06-infinity-findings

4 stars 0 forks source link

Extra msg.value in `takeOrders` #343

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L361-L363

Vulnerability details

Impact

In takeOrders, msg.sender can pay the items in ether (or other native coins).

    // check to ensure that for ETH orders, enough ETH is sent
    // for non ETH orders, IERC20 safeTransferFrom will throw error if insufficient amount is sent
    if (isMakerSeller && currency == address(0)) {
      require(msg.value >= totalPrice, 'invalid total price');
    }

However the extra payment isn't returned to the caller. This can be a risk considering that they may send extra for items with "moving" price. Also they may mistakenly send ether to the contract when it's not needed. Indeed the function doesn't revert in that case.

Proof of Concept

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/core/InfinityExchange.sol#L361-L363

Recommended Mitigation Steps

Add a else with a require(msg.value==0). Also consider returning any extra value to msg.sender.

KenzoAgada commented 2 years ago

Duplicate of #244 (extra ether not returned) and #346 (ether might be sent along with erc20)

HardlyDifficult commented 2 years ago

Duping to https://github.com/code-423n4/2022-06-infinity-findings/issues/346

It does also touch on https://github.com/code-423n4/2022-06-infinity-findings/issues/244 but the impact/poc/rec wasn't fully explored for that issue.