QuickSwap and StellaSwap contest - 0xDecorativePineapple's results

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/libraries/TransferHelper.sol#L21-L22

Vulnerability details

Impact

In TransferHelper.sol the safeTransfer function is as follows:

 function safeTransfer(
    address token,
    address to,
    uint256 value
  ) internal {
    (bool success, bytes memory data) = token.call(abi.encodeWithSelector(IERC20Minimal.transfer.selector, to, value));
    require(success && (data.length == 0 || abi.decode(data, (bool))), 'TF');
  }

This function is utilized in a few different places in the contract. According to the Solidity docs),

The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

As a result, if the tokens have not yet been deployed or have been destroyed, safeTransfer will return success even though no transfer was executed. If the token has not yet been deployed, no liquidity can be added. However, if the token has been destroyed, the pool will act as if the assets were sent even though they were not.

Proof of Concept

The pool contains tokens X and Y. Token X has a bug, and the contract is destroyed. Bob is not aware of the issue and swaps 10_000 Y tokens for X tokens. Bob successfully transfers 10_000 Y tokens to the pool but does not receive any X tokens in return. As a result, Bob loses 10_000 Y tokens.

Tools Used

Manual Code Review

Recommended Mitigation Steps

It is recommended to check the contract’s existence prior to the low-level call in TransferHelper.safeTransfer. This will ensure that a swap reverts if the token to be bought no longer exists, preventing the pool from accepting the token to be sold without returning any tokens in exchange.

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L479 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L548 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L906

Vulnerability details

Impact

The Algebra protocol does not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.

Proof of Concept

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L479 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L548 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L906

Tools Used

Manual Code Review

Recommended Mitigation Steps

• Make sure token vault accounts for any rebasing/inflation/deflation
• Add support in contracts for such tokens before accepting user-supplied tokens
• Consider to check before/after balance on the vault.

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L193

Vulnerability details

Impact

A front-run on the initialize function of the Algebra protocol allows an attacker to set an unfair price and to drain assets from the first deposits.

Bellow is the initialize function:

  function initialize(uint160 initialPrice) external override {
    require(globalState.price == 0, 'AI');
    // getTickAtSqrtRatio checks validity of initialPrice inside
    int24 tick = TickMath.getTickAtSqrtRatio(initialPrice);

    uint32 timestamp = _blockTimestamp();
    IDataStorageOperator(dataStorageOperator).initialize(timestamp, tick);

    globalState.price = initialPrice;
    globalState.unlocked = true;
    globalState.tick = tick;

    emit Initialize(initialPrice, tick);
  }

There are no access controls on the function, so anyone could call it on a deployed pool. Initializing a pool with an incorrect price allows an attacker to generate profits from the initial liquidity provider’s deposits.

Proof of Concept

• Bob deploys a pool for assets X and Y through a deployment script. The current market price is 1 X == 1 Y.
• Eve front-runs Bob’s transaction to the initialize function and sets a price such that 1 X ~= 10 Y.
• Bob calls mint and deposits assets X and Y worth $100,000, sending ~10 times more of asset Y than asset X.
• Eve swaps X tokens for Y tokens at an unfair price, profiting off of Bob’s deployment.

Tools Used

Manual Code Review

Recommended Mitigation Steps

It is recommended to consider:

• moving the price operations from initialize to the constructor,
• adding access controls to initialize, or
• ensuring that the documentation clearly warns users about incorrect initialization.

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraFactory.sol#L77-L81

Vulnerability details

Impact

In the AlgebraFactory.sol smart contract of the Algebra Protocol, the ownership can be transferred to a new owner via the setOwner function. The owner can then perform several privileged actions, such as setting the farmingAddress and setting the vaultAddress .The impact is that, if an incorrect address (for example and address for which the private key is not known) is used accidentally, then it blocks several functionalities of the protocol.

Also, due to the fact that zero-address check isn't implemented in the setOwner function, if the current owner calls that function but mistakenly passes address(0) as the _owner the ownership will be lost forever.

You could see See similar High Risk severity finding from Trail-of-Bits Audit of Hermez and similar Medium Risk severity finding from Trail-of-Bits Audit of Uniswap V3.

Proof of Concept

You could find bellow the implementation of the setOwner function.

  function setOwner(address _owner) external override onlyOwner {
    require(owner != _owner);
    emit Owner(_owner);
    owner = _owner;
  }

Tools Used

Manual Code Review

Recommended Mitigation Steps

It is recommended to implement a 2-step function in order to transferOwnership to a new owner. The first function will approve a new address as the pendingOwner. The second function, which is only callable by the pendingOwner will claim the pending Owner and will transfer the full ownership to the pendingOwner.

If it needs to be possible to set the owner to address(0), it is advised to implement a renounceOwnership function