turvec - Protocol will loss all fees due to injected fees not being reflected on trader orders

[sherlock-admin2]

turvec

high

Protocol will loss all fees due to injected fees not being reflected on trader orders

Summary

Protocol will loss all fees due to injected fees not being reflected on trader orders

Vulnerability Detail

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L213 In the code snipet provided above, the changes made to the order parameter within the _injectFees function will not reflect on the original orders array passed into the _prepare function. This is because the order parameter is passed by value to the _injectFees function, and any modifications made to it are only affecting the local copy, not the original array element.

In Solidity, parameters are always passed by value, and for complex types like structs and arrays, a new copy is made. Changes to the copy do not affect the original data unless you explicitly return the modified copy and reassign it to the original.

Proof Of Concept

function _prepare(ResolvedOrder[] memory orders) internal {
        uint256 ordersLength = orders.length;
        unchecked {
            for (uint256 i = 0; i < ordersLength; i++) {
                ResolvedOrder memory order = orders[i];
                _injectFees(order); // @audit-issue
                order.validate(msg.sender);
                transferInputTokens(order, msg.sender);
            }
        }
    }
    ---
    function _injectFees(ResolvedOrder memory order) internal view {
       ...

        order.outputs = newOutputs;
    }

Here's a breakdown of what happens in the code:

1. ResolvedOrder memory order = orders[i]; creates a copy of the i-th element of the orders array.
2. _injectFees(order); passes the copy to the _injectFees function, which modifies the copy.
3. The modified copy is not automatically synced back to the original orders array.

Impact

The modified copy is not automatically synced back to the original orders array causing protocol to loss all fees due to injected fees not being reflected on trader orders.

Code Snippet

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/reactors/BaseGladiusReactor.sol#L213 https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/base/ProtocolFees.sol#L39 https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/base/ProtocolFees.sol#L104

Tool used

Manual Review

Recommendation

To ensure that the changes made to order in _injectFees are reflected in the original orders array, you would need to return the modified order from _injectFees and then reassign it to the corresponding element in the orders array. Here's how you could adjust the contract:

function _prepare(ResolvedOrder[] memory orders) internal {
    for (uint256 i =  0; i < orders.length; i++) {
        ResolvedOrder memory order = orders[i];
-      _injectFees(order);
+      order = _injectFees(order); // Reassign the modified order back to the local copy
        order.validate(msg.sender);
        transferInputTokens(order, msg.sender);
    }
}

However for this to work, you have to modify the _injectFees() function in the ProtocolFees.sol contract as well

- function _injectFees(ResolvedOrder memory order) internal view {
+ function _injectFees(ResolvedOrder memory order) internal view returns (ResolvedOrder memory) {
    ...
    order.outputs = newOutputs;
+  return order; // Return the modified order
}

Notice that the _injectFees function now has a return type and returns the modified order. In the _prepare function, the returned order is reassigned to the local copy, ensuring that fee injected persist.

Duplicate of #76

[sherlock-admin]

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

0xAadi commented:

Invalid: _injectFees(order) update the order, as the order utilizes the memory reference of the element within the struct array orders.