Basin - peanuts's results

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Aquifer.sol#L46-L51

Vulnerability details

Impact

DoS for the Aquifer.boreWell() function due to frontrunning.

Proof of Concept

From the video documentation, Anyone can call boreWell() in Aquifer.sol after confirming an implementation contract. The address of the new Well depends solely upon the _salt parameter provided by the user. Once the user's create transaction is broadcasted, the _salt parameter can be viewed by anyone watching the public mempool.

As a result, if a user intends to permissionlessly create a well, his create transaction can be forcefully reverted by an attacker by deploying a well for himself using the user's salt. Here is how this can happen:

• The user broadcasts the create well txn with salt XXX.
• The attacker frontruns the user's txn and creates a well for himself using the same XXX salt.
• The user's original create txn gets reverted as attacker's well already exist on the predetermined address.
• This attack can be repeated again and again resulting in DoS for the Aquifer.boreWell() function.

        } else {
            if (salt != bytes32(0)) {
@>              well = implementation.cloneDeterministic(salt);
            } else {
                well = implementation.clone();
            }

Tools Used

VSCode

Recommended Mitigation Steps

Consider making the upcoming well address user specific by combining the salt value with user's address.

        } else {
            if (salt != bytes32(0)) {
+               well = implementation.cloneDeterministic(msg.sender, salt);
            } else {
                well = implementation.clone();
            }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L215-L240

Vulnerability details

Impact

Slippage may not be accounted for accurately.

Proof of Concept

Well.sol accepts fee-on-transfer tokens, as seen in the NATSPEC and the functions.

    function swapFromFeeOnTransfer(

The feeOnTransfer token can be either the incoming token or the outgoing token or both. Let's focus on the outgoing token. When a user wants to swap tokenX to tokenY (take tokenX as fee-on-transfer), he calls swapFromFeeOnTransfer() which calls _swapFrom(). _swapFrom() does the necessary checks and updates before calculating the amountOut. The function then checks that amountOut must be greater than minAmountOut before transferring the amountOut value to the user.

        amountOut = reserveJBefore - reserves[j];
        if (amountOut < minAmountOut) {
            revert SlippageOut(amountOut, minAmountOut);
        }


        toToken.safeTransfer(recipient, amountOut);
        emit Swap(fromToken, toToken, amountIn, amountOut, recipient);

However, the fee for transferring the amountOut to the user is not accounted in the minAmountOut variable, which means that the fee for transferring is not included in the slippage.

For example, if tokenY has a 10% transfer fee.

• Alice wants to swap 100 tokenX for a desired 100 tokenY.
• Her minAmount received for tokenY is 95.
• When swapping, the initial amount that she will be getting before fees is 97 tokenY.
• Since her amount out, 97, is greater than min amount, 95, the slippage check will pass.
• However, during the transfer, since the percentage is 10%, Alice will only get ~87 tokens, which is lesser than her minAmount. The slippage check fails to consider the fee when transferring tokens.

Tools Used

VSCode

Recommended Mitigation Steps

When interacting with fee on transfer tokens, the slippage check should be done after the transfer to account for the transfer fee.

        amountOut = reserveJBefore - reserves[j];
-       if (amountOut < minAmountOut) {
-           revert SlippageOut(amountOut, minAmountOut);
-       }

       
-       toToken.safeTransfer(recipient, amountOut);

+       uint256 balanceBefore = toToken.balanceOf(recipient);
+       token.safeTransfer(recipient, amountOut);
+       amountTransferred = toToken.balanceOf(recipient) - balanceBefore;      

+       if (amountTransferred < minAmountOut) {
+           revert SlippageOut(amountOut, minAmountOut);
+       }


        emit Swap(fromToken, toToken, amountIn, amountOut, recipient);

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/main/src/Well.sol#L22 https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L31-L34

Vulnerability details

Impact

Reentrancy Guard will not work as intended.

Proof of Concept

Well.sol uses ReentrancyGuardUpgradeable.sol from OpenZeppelin.

import {ReentrancyGuardUpgradeable} from "ozu/security/ReentrancyGuardUpgradeable.sol";

However, it is not initialized.

    function init(string memory name, string memory symbol) public initializer {
        __ERC20Permit_init(name);
        __ERC20_init(name, symbol);

In OpenZeppelin's ReentrancyGuardUpgradeable, __ReentrancyGuard_init() should be called:

    function __ReentrancyGuard_init() internal onlyInitializing {
        __ReentrancyGuard_init_unchained();
    }


    function __ReentrancyGuard_init_unchained() internal onlyInitializing {
        _status = _NOT_ENTERED;
    }

If __ReentrancyGuard_init() is not called, the modifier nonReentrant will not work as intended.

Tools Used

Manual Review

Recommended Mitigation Steps

Call __ReentrancyGuard_init() in init() in Well.sol.

    function init(string memory name, string memory symbol) public initializer {
        __ERC20Permit_init(name);
        __ERC20_init(name, symbol);
+       __ReentrancyGuard_init() 

Assessed type

Upgradable

Table of Contents

1. Context
2. Approach
3. Mechanism Review
4. Architecture Recommendations
5. Systemic Risks

1. Context

Spent most of the time auditing the Well's code and briefly looked through the Math in the Pump contract.

2. Approach

1. Read the two whitepapers and watch the video documentation first.
2. Try to understand how the Well and the Pump works together by looking at the high-level diagram given.
3. Look at each code individually and focus on the internal function calls.
4. Read the test files to get a better idea of the end-to-end scenarios.

3. Mechanism Review

1. The addition of both 'swapExactInputSingle' (swapFrom) and 'swapExactOutputSingle' (swapTo) style when swapping the tokens is appreciated and well created, especially since the latter is not very common.

2. SwapTo() calculates the input amount before transferring the tokens in. This is unlike how Uniswap does it, which is transferring the maximum amount of tokens in and then refunding the remaining tokens back. The protocol does it better and saves some gas in the process as well by doing just one transfer.

3. Shift is an interesting concept as it opts away from using the router and let the Wells interact with each other instead. Saves some gas by not transferring tokens around many times.

4. Deadline, slippage is set properly in every swap and increase/decrease liquidity.

5. Getter functions are well written and plenty; users have access to many external view functions to get a sense of what they are doing before calling the actual function.

4. Architecture Recommendations

1. I recommend that the swap functions (swapFrom() and swapFromFeeOnTransfer()) should have a bool feeOnTransfer flag the same as addLiquidity() in order to save gas in case the user accidentally calls the wrong function. This saves the user some gas in case he wants to use fee-on-transfer token but calls swapFrom() instead, and then has to wait until _setReserves() is calculated before the function reverts, which costs quite a bit of gas for the computation. Should check whether the swap uses fee-on-transfer tokens from the very start as such:

    function swapFrom(
        IERC20 fromToken,
        IERC20 toToken,
        uint256 amountIn,
        uint256 minAmountOut,
        address recipient,
        uint256 deadline,
+       bool isFeeOnTransfer
    ) external nonReentrant expire(deadline) returns (uint256 amountOut) {
+       if(isFeeOnTransfer) revert();
        fromToken.safeTransferFrom(msg.sender, address(this), amountIn);

2. The swapTo function can also include the feeOnTransfer parameter (must always be false) to save some gas in case the user unknowningly uses the swapTo() function with a fee-on-transfer token.

3. Since the Well function is awaiting a proper implementation, protocol could give an example of how to edit the Well contract. For example, when updating pump, the protocol can give a possible commented code block if an external shutoff mechanism were to be added:

            try IPump(_pump.target).update(reserves, _pump.data) {}
            catch {
                // ignore reversion. If an external shutoff mechanism is added to a Pump, it could be called here.
+           // Example of how a shutoff mechanism would look like in code
 }

5. Systemic Risks

Since the creation of the Well clone is permissionless, the implementation contract has to be airtight to make sure that the creators of the well cannot exploit the well through inflation attacks or any sort of attacks when minting LPs, swapping or decreasing liquidity. This poses a huge systemic risk because if the implementation fails, every clone that clones through the foiled implementation will also be at risk.

Also, even though the Wells are permissionless, if any Well user is malicious (if there is an opportunity to be malicious), then it will affect the reputation of the protocol since the whole system relies on the Well contracts.

Time spent:

15 hours