Critical address changes should be a two step process

[code423n4]

Handle

Ruhum

Vulnerability details

Impact

The contracts mostly use OpenZeppelin's Ownable contract. The contract handles the address change in a single step.

If by chance the address passed to transferOwnership() function has a typo, the owner would lose access to the whole deployed contract. In that case, the contract would have to be redeployed.

Instead, it is recommended to use a two-step process. First, you set the new address as the current owner. Then you have to claim the ownership with the new address. Thus, you can be sure that the new address is valid and you have access to it. That way you won't accidentally lose control over your contracts.

Here's Sushiswap's implementation of the Ownable contract as an example:
https://github.com/sushiswap/sushiswap/blob/4fdfeb7dafe852e738c56f11a6cae855e2fc0046/contracts/Ownable.sol

Proof of Concept

OpenZeppelin's Ownable: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L62

Swap.sol used OwnerPausable: https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/Swap.sol#L30 which is defined here: https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/OwnerPausable.sol

Tools Used

Manual Analysis

Recommended Mitigation Steps

Use a two-step process for critical address changes as seen here: https://github.com/sushiswap/sushiswap/blob/4fdfeb7dafe852e738c56f11a6cae855e2fc0046/contracts/Ownable.sol

[chickenpie347]

Duplicate of #35