QuickSwap and StellaSwap contest - catchup's results

Lines of code

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

Vulnerability details

Impact

initialize() is an external function which sets the globalState.price with an initial value. This function should only be called by the owner, once a new pool is deployed. However, anyone can frontrun the owner and can set the initial price to an unfair value, since there is no access control. Then once an early user deposits in the pool, malicious user can take advantage of the faulty swap ratio to make profit off the early user.

193:   function initialize(uint160 initialPrice) external override {		//@audit-issue there is no modifier control. Anyone can set the initialprice.
194:     require(globalState.price == 0, 'AI');
195:     // getTickAtSqrtRatio checks validity of initialPrice inside
196:     int24 tick = TickMath.getTickAtSqrtRatio(initialPrice);
197: 
198:     uint32 timestamp = _blockTimestamp();
199:     IDataStorageOperator(dataStorageOperator).initialize(timestamp, tick);
200: 
201:     globalState.price = initialPrice;
202:     globalState.unlocked = true;
203:     globalState.tick = tick;
204: 
205:     emit Initialize(initialPrice, tick);
206:   }

Proof of Concept

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

Tools Used

Manual review

Recommended Mitigation Steps

I suggest to include an access control on the initialize() function, so that it can only be called by the owner.

Lines of code

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

Vulnerability details

Impact

Current owner can update the owner address by calling setOwner(address _owner) function:

77:   function setOwner(address _owner) external override onlyOwner {
78:     require(owner != _owner);   //@audit-issue uncontrolled owner update
79:     emit Owner(_owner);
80:     owner = _owner;
81:   }

Owner has important privileges such as setting the farming and vault addresses, setting the base feeconfiguration, updating the community fees and updating the liquidity cooldown time. If the current owner updates the new owner address by passing a faulty address, all these privileges would be lost and the owner address would not be able to be recovered again, since only the current owner can update the owner address.

While this kind of zero address checks are considered as QA among the judges, a similar submission was accepted as Medium recently (find the link below): https://github.com/code-423n4/2022-08-nounsdao-findings/issues/318

Proof of Concept

https://github.com/code-423n4/2022-08-nounsdao-findings/issues/318

Tools Used

Manual review

Recommended Mitigation Steps

I suggest to include a 2-step process for the owner updates, where the current owner would set the pendingAdmin address and the pendingAdmin would claim the owner address.