QuickSwap and StellaSwap contest - kaden's results

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/libraries/TransferHelper.sol#L21 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L478 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L505 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#L604 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L610 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L658 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L906 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L913 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L929 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L943

Vulnerability details

Description:

AlgebraPool uses TransferHelper.safeTransfer to execute token transfers. safeTransfer performs a low-level call to the token contract, executing the transfer method. However, low-level calls in solidity will always return success if the calling account is non-existent. As a result, calls using this safeTransfer method may falsely succeed and continue execution of the method with a failed token transfer in the case that the token contract has been self destructed or simply does not exist.

Remediation:

It is recommended that either:

• A check is added to ensure the contract being called exists, or
• A high level transfer call is used in place of the low-level call.

Earned interest to tokens in pool can be stolen

Severity: Low

Context:

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

Description:

In AlgebraPool.swap, the token balance of the contract is called before and after the _swapCallback is executed to enforce the user to transfer an appropriate amount of tokens to the contract. It's possible that in the case of a non-standard ERC-20, an attacker may call a method on the token to accrue interest to the pool contract, such that the balance increases sufficiently for them to execute their swap without actually transferring any tokens.

Remediation:

It is recommended that the possible effects of non-standard ERC-20 tokens are well documented to minimize loss of user funds.

Balance checks can be optimized to avoid redundant extcodesize check

Severity: Gas optimization

Context:

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

Description:

High-level balanceOf methods to check token balances make use of a redundant extcodesize check and can be optimized as, e.g.:

(bool success, bytes memory data) =
    token0.staticcall(abi.encodeWithSelector(IERC20Minimal.balanceOf.selector, address(this)));
require(success && data.length >= 32);
return abi.decode(data, (uint256));

The factory contract can inherit the deployer contract to remove unecessary logic

Severity: Gas optimization

Context:

• https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPoolDeployer.sol#L49
• https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPoolDeployer.sol#L21:L24

Description:

The AlgebraPoolFactory contract is intended to be the only allowed executor of AlgebraPoolDeployer.deploy, because of this, checks are included to ensure that the sender is the factory contract. Redundant logic can be removed by having the factory contract inherit the deployer contract and making AlgebraPoolDeployer.deploy internal.

Parameters can be safely deleted after pool deployment

Severity: Gas optimization

Context:

• https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPoolDeployer.sol#L44:L53

Description:

AlgebraPoolFactory.deploy shares the parameters with the AlgebraPool instance. However, since these parameters are only used in the pool contract's constructor, they can be deleted once the pool is created, reducing gas consumption.