Retrieving Token Balances with balanceOf is dangerous

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

MagicLP.sol contract relies on the balanceOf function of the ERC20 tokens to keep track of the reserves within the contract. It uses the actual token balances for internal accounting rather than maintaining a separate ledger for sellBase and sellQuote function. Retrieving Token Balances with balanceOf is dangerous as balanceOf may be gamed. Future liquidity providers and traders will be affected because the pool's price and reserve ratios have been manipulated, potentially leading to arbitrage opportunities or loss of funds.

Proof of Concept

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

function _sync() internal {
        uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
        uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));

        if (baseBalance != _BASE_RESERVE_) {
            _BASE_RESERVE_ = baseBalance.toUint112();
        }
        if (quoteBalance != _QUOTE_RESERVE_) {
            _QUOTE_RESERVE_ = quoteBalance.toUint112();
        }

        _twapUpdate();
    }

The above _sync function retrieve Token Balances by calling balanceOf function on the ERC20 token contract for both the base and qoute tokens. Then checks if the retrieved balances differ from the contract's internal reserve balances (_BASE_RESERVE_ and _QUOTE_RESERVE_) to update reserves. Finally, after updating the reserve balances, the function calls _twapUpdate to update the Time-Weighted Average Price (TWAP) of the base token.

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L545 The _twapUpdate function in the MagicLP.sol contract is responsible for updating the Time-Weighted Average Price (TWAP) of the base token.

 function _twapUpdate() internal {
        uint32 blockTimestamp = uint32(block.timestamp % 2 ** 32);
        uint32 timeElapsed = blockTimestamp - _BLOCK_TIMESTAMP_LAST_;

        if (timeElapsed > 0 && _BASE_RESERVE_ != 0 && _QUOTE_RESERVE_ != 0) {
            /// @dev It is desired and expected for this value to
            /// overflow once it has hit the max of `type.uint256`.
            unchecked {
                _BASE_PRICE_CUMULATIVE_LAST_ += getMidPrice() * timeElapsed;
            }
        }

Hence, _sync() in MagicLP contract relies on the balanceOf function of the ERC20 tokens to keep track of the reserves within the contract. It uses the actual token balances for internal accounting rather than maintaining a separate ledger for sellBase and sellQuote function. Retrieving Token Balances with balanceOf is dangerous as balanceOf may be gamed. This is evident in the _sync function and in sellBase, sellQoute where the contract updates its reserve state: sellBase, sellQoute https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L262 https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L285

Exploit scenario

Alice and Bob are liquidity providers in the MagicLP contract. The MagicLP contract has 1000 BASE and 1000 QUOTE tokens in reserves.

Bob decides to attack the contract by sending 100 BASE tokens directly to the MagicLP contract address without using any function that updates the internal reserves. Now the actual token balance of the MagicLP contract is 1100 BASE and 1000 QUOTE, but the internal reserves are still 1000 BASE and 1000 QUOTE.

Alice calls sellBase to swap some of her BASE tokens for QUOTE tokens. The sellBase function checks the contract's BASE token balance, which includes the extra 100 BASE tokens Bob sent. The function calculates the amount of QUOTE tokens Alice should receive based on the inflated BASE balance. Alice receives more QUOTE tokens than she should, according to the original reserves, because the contract's calculations are based on the actual token balance, which has been manipulated.

The MagicLP contract's internal accounting is now out of sync with the actual token balances. Alice receives more QUOTE tokens than she should, depleting the QUOTE reserves unfairly. Future liquidity providers and traders are affected because the pool's price and reserve ratios have been manipulated, potentially leading to arbitrage opportunities or loss of funds.

Tools Used

Manual Review and Solidity Youtube Tutorial

Recommended Mitigation Steps

Implement internal accounting instead of relying on balanceOf natively properly.

Assessed type

Other

[0xCalibur]

Please provide a POC using a fork testing where this viable exploit and profitable. This is by design and users should be using the Router with min output amount.

We'll reconsider upon a POC where the user is using the intended router with min output amount.

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as duplicate of #96

[c4-pre-sort]

141345 marked the issue as remove high or low quality report

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid