When a trader swaps from a smart contract wallet, anyone could make them lose additional value through the trade.

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L250

Vulnerability details

Description

The MagicLP contract implements swapping through the sellBase() and sellQuote() functions. It calculates the out amount based on the input amount and the tx.origin:

 (receiveQuoteAmount, mtFee, newRState, newBaseTarget) = querySellBase(tx.origin, baseInput);

The origin is passed as trader to the getFeeRate() function to get the custom lpFeeRate, mtFeeRate for that user:

function querySellBase(
    address trader,
    uint256 payBaseAmount
) public view returns (uint256 receiveQuoteAmount, uint256 mtFee, PMMPricing.RState newRState, uint256 newBaseTarget) {
    PMMPricing.PMMState memory state = getPMMState();
    (receiveQuoteAmount, newRState) = PMMPricing.sellBaseToken(state, payBaseAmount);
    // Passed here @@@@@@@
    (uint256 lpFeeRate, uint256 mtFeeRate) = _MT_FEE_RATE_MODEL_.getFeeRate(trader, _LP_FEE_RATE_);
    mtFee = DecimalMath.mulFloor(receiveQuoteAmount, mtFeeRate);
    receiveQuoteAmount = receiveQuoteAmount - DecimalMath.mulFloor(receiveQuoteAmount, lpFeeRate) - mtFee;
    newBaseTarget = state.B0;
}

The idea to use tx.origin is due to the expected interaction with the contract through the Router periphery contract, in this case the msg.sender is not an accurate representation of the trader.

The issue is that when the trade is executed from a smart contract wallet, for example Gnosis Safe, tx.origin is at the control of the attacker. They may observe the execTransaction() call in the mempool and submit it by themselves, copying the parameters including the signatures. Control of the tx.origin grants the attacker control over the lpFeeRate and mtFeeRate charged in the TX. This means that the victim could assume a certain fee for the swap, while they would be overcharged due to an attacker frontrunning the TX. Note that this attack works both on direct interactions with the MagicLP or TXs through the Router contract.

Impact

When a trader swaps from a smart contract wallet, anyone could make them lose additional value through the trade.

Proof of Concept

1. A smart contract wallet executes a swap through Router's sellBaseTokensForTokens()
2. An attacker views the call in the mempool and executes the call from their own EOA / smart contract.
3. The registered tx.origin for fee determination will be the attacker instead of the real sender.
4. Extra fees will be absorbed by the protocol leading to loss of value for the trader.

Severity Rationalization

• The prerequisite that victim must be a smart contract wallet is not a limiting condition, because it is extremely likely smart contract wallets, or smart contracts integrating with MIMswap, will be used.
• The FeeRateModel implementation example ignores the trader passed in the second argument. However that is not valid grounds for downgrading since the issue is in the MagicLP contract, which cannot make any assumptions on the non-usage of any passed parameters. It is very reasonable to assume the trader parameter would be used, otherwise it wouldn't be passed to the function in a newly-written codebase.

Without severity reduction from above it seems clear the loss of capital translates to high-severity.

Tools Used

Manual audit

Recommended Mitigation Steps

Abolish the use of tx.origin. The Router can pass the msg.sender down to the MagicLP contract to determine the intended trader.

Note that the issue extends also to the flashloan() function, to a lesser impact.

Assessed type

Invalid Validation

[0xCalibur]

It's by design and we don't plan to be using the trader parameter but If we were to use it we would take extra precautious by whitelisting allowed addresses.

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[141345]

tx.origin exploit need to take a close look at the POC

[c4-pre-sort]

141345 marked the issue as primary issue

[thereksfour]

The victim is smart wallet reduces its likelihood, and the issue has predictions of future code which further reduces its likelihood and impact. All things considered I would downgrade it to QA

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)

[trust1995]

Hi,

The baseline impact we are dealing with is loss of funds due to paying more fees than intended (High). I have made a case why speculation of future code is valid in this case in the description, but I agree it decreases likelihood somewhat (It does not decrease impact though). I don't think use of smart contract wallets should be out of scope since the contracts never check it and there aren't any remarks around that option anywhere. Users should be free to use whatever compliant wallet they would like while working with the contract safely. All things considered it seems the lowest the finding could get downgraded to is Medium.

[thereksfour]

I still think predicting future code will reduce the impact of the issue, or it will directly reduce the severity of the issue.