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

5 stars 3 forks source link

detectiveking - `getFeeOutputs` fees are improperly calculated #43

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

detectiveking

high

getFeeOutputs fees are improperly calculated

Summary

for getFeeOutputs in RubiconFeeController, fees are improperly calculated, leading to loss of fees for the fee recipient

Vulnerability Detail

In getFeeOutputs, there exists the following code:

                for (uint256 j = 0; j < feeCount; ++j) {
                    OutputToken memory feeOutput = result[j];

                    if (feeOutput.token == tokenOut) {
                        found = true;
                        feeOutput.amount += feeAmount;
                    }
                }

Because feeOutput is a memory variable, however, the feeOutput amount is not actually updated here (in the event that the token already exists in result). It should instead be storage so that the feeOutput is updated. This leads to under-calculation of fees, leading to the fee recipient losing out on fees.

Impact

Fee recipient receiving less fees than expected

Code Snippet

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/fee-controllers/RubiconFeeController.sol#L63

Tool used

Manual Review

Recommendation

Change memory to storage

Duplicate of #64

sherlock-admin commented 7 months ago

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

0xAadi commented:

Invalid: updating feeOutput update results array, becase it utilizes the memory reference of the element