Dobry - The `_fill` function allows user to steal funds that do not belong to them

[sherlock-admin]

Dobry

medium

The _fill function allows user to steal funds that do not belong to them

medium

Summary

Users can steal funds that do not belong to them from the smart contract.

Vulnerability Detail

The _fill function is responsible for filling the list of orders, however at its bottom the whole smart contract balance is being sent to the msg.sender.

        if (address(this).balance > 0) {
            CurrencyLibrary.transferNative(msg.sender, address(this).balance);
        }

The transferNative function executes a low level call to the msg.sender, sending the passed parameter as an amount, which in our case is all of the funds in the smart contract.

Impact

This will let any user collect all of the funds in the smart contact, even if all of the funds/part of the funds do not belong to them. Consider an user sending funds to the smart contract. They will be collected by another user calling any of the execute, executeBatch, executeWithCallback or executeBatchWithCallback functions due to the fact that all of them call the _fill function.

Code Snippet

The _fill function:

222:    function _fill(ResolvedOrder[] memory orders) internal {
        .
        .
        .
248:        if (address(this).balance > 0) {
249:            CurrencyLibrary.transferNative(msg.sender, address(this).balance);
250:        }
251:   }

The transferNative function:

        function transferNative(address recipient, uint256 amount) internal {
            (bool success, ) = recipient.call{value: amount}("");
            if (!success) revert NativeTransferFailed();
        }

Tool used

Manual Review

Recommendation

Make additional calculations to make sure that the msg.sender receives only the funds that belong to them.

[sherlock-admin]

2 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

This shouldn't be a problem because the contract won't keep funds

0xAadi commented: