code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

The setOwner function from the AlgebraFactory contract is missing a check for address(0) when setting a new owner which could lead to the loss of the ownership of the contract if the old owner passes _owner == address(0) by accident.

The impact of this is that the FarmingAddress, VaultAddress and BaseFeeConfiguration variables can not be changed anymore which can cause the protocol to not work correctly, and because the factory address can not be modified from the AlgebraPoolDeployer contract the developers will not be able to deploy another factory contract.

Proof of Concept

Instance occurs in : function setOwner(address _owner)

As you can see with the setOwner function the current owner can easily burn the ownership by passing address(0) :

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

Tools Used

Manual audit

Recommended Mitigation Steps

There are 3 options to fix this issue :

The safer option is to use openzeppelin Ownable extension to manage ownership & access control.
Use a two-step ownership transfer process in order to avoid giving ownership to a wrong address or zero address by accident, because with the two-step ownership the new owner must approve the transfer.
The simple option is to add non-zero address check on the new owner address in the setOwner function to avoid burning ownership by accident.

sameepsi commented 2 years ago

duplicate of #131

0xean commented 2 years ago

downgrading - dupe of #269 - wardens QA report

code-423n4 / 2022-09-quickswap-findings