msg.value inside a loop can cause to disruption on the orders

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L335 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L546

Vulnerability details

Impact

During the code review, It has been observed that batchFillOrder function uses msg.value over the for loop. In Putty Order handling, msg.value is used inside a loop in a payable function. If fillOrder() is called with multiple receivers, the same msg.value will be reused for each recipient even though the corresponding ETH for only one recipient is sent.

Proof of Concept

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L546

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L335

1. msg.value is a global variable and if you use it in a loop (batch), you may get unexpected results. If you have a function similar to batch, make sure to check the increase in balance manually rather than using the msg.value directly.

Tools Used

Code Review

Recommended Mitigation Steps

The semantics of Ethereum is counterintuitive. The usage of msg.value within top-level calls is fairly easy to understand. However, once the usage is contained within an internal call or a delegatecall, the specified ETH amount might have been accounted for already. Review msg.value usage.

Reference : https://mudit.blog/miso-war-room/

[berndartmueller]

The batchFillOrder function is not payable, any usage of msg.value within this loop is without effects. I think that was the reason that no payable modifier was added to the batchFillOrder function, to prevent exactly this issue.

[outdoteth]

Can confirm what @berndartmueller said. batchFillOrder cannot handle native ETH for this reason. The documentation should have been better here - sorry.

[HickupHH3]

dup of #138