Avoiding the Use of address(this).balance in BaseGladiusReactor Contract

Summary

The BaseGladiusReactor contract utilizes address(this).balance to refund remaining Ether to the msg.sender. However, this approach may lead to unexpected behavior and security risks, as the funds may not exclusively belong to the msg.sender.

Vulnerability Detail

Using address(this).balance to determine refund amounts and recipients assumes that all Ether held by the contract belongs to the msg.sender. This assumption can be incorrect, especially in contracts that interact with multiple users or receive Ether from various sources.

Impact

Misattribution of Funds: There is a risk of misattributing funds if the contract receives Ether from different sources, leading to incorrect refunds.
Security Vulnerabilities: Relying on address(this).balance can introduce security vulnerabilities, such as reentrancy attacks or unexpected interactions with other contracts sharing the same balance.
Contract Upgradability: Assumptions about Ether ownership may become outdated in upgradeable contracts, potentially causing unintended consequences during upgrades or migrations.

Code Snippet

function _fill(ResolvedOrder[] memory orders) internal {
    // Transfer output tokens to their respective recipients
    for (uint256 i = 0; i < orders.length; i++) {
        ResolvedOrder memory resolvedOrder = orders[i];
        uint256 outputsLength = resolvedOrder.outputs.length;
        for (uint256 j = 0; j < outputsLength; j++) {
            OutputToken memory output = resolvedOrder.outputs[j];
            output.token.transferFill(output.recipient, output.amount);
        }
        emit Fill(
            orders[i].hash,
            msg.sender,
            resolvedOrder.info.swapper,
            resolvedOrder.info.nonce
        );
    }

    // Refund any remaining ETH to the filler
    if (address(this).balance > 0) {
        CurrencyLibrary.transferNative(msg.sender, address(this).balance);
    }
}

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L248-L250

Tool used

Manual Review

Recommendation

Implement explicit accounting mechanisms to track the ownership and allocation of Ether within the contract.
Maintain separate balances or accounting records for different users or purposes to accurately determine refund amounts and recipients.
Design secure refund mechanisms that specify the refund amount and recipient based on transaction-specific parameters or user preferences, rather than relying on the overall contract balance.
Clearly communicate the refund process and any associated conditions or limitations to users to manage expectations and prevent misunderstandings.

sherlock-audit / 2024-02-rubicon-finance-judging

calpaliu - Avoiding the Use of address(this).balance in BaseGladiusReactor Contract #11