Udsen - The protocol heavily depends on admin actions, hence single-step ownership transfer is dangerous

[github-actions[bot]]

Udsen

medium

The protocol heavily depends on admin actions, hence single-step ownership transfer is dangerous

Summary

The PerpDepository.sol function heavily depends on the onlyOwner admin actions and critical functions of the contract depends on the onlyOwner as well. Hence the transfering of ownership should be done in two step rather than in a single step for security purpose.

Vulnerability Detail

The transferOwnership(address newOwner) function calls the super.transferOwnership() function of the OpenZeppelin OwnableUpgradeable which only checks for the zero address condition. Hence by mistake an erroneous address can be confirmed as the newOwner.

Impact

Here the single-step ownership transfer pattern is used. If by mistake an admin provides an incorrect address for the new owner, then none of the onlyOwner methods will be callable again. The recommended solution will be to use the two-step ownership transfer pattern. Using the two-step process the new owner will have to first claim the ownership and then the ownership will be transferred to him.

Code Snippet

https://github.com/sherlock-audit/2023-01-uxd/blob/main/contracts/integrations/perp/PerpDepository.sol#L846-L848

function transferOwnership(address newOwner) public override(IDepository, OwnableUpgradeable) onlyOwner {
    super.transferOwnership(newOwner);
}

Tool used

Manual Review

Recommendation

Use Ownable2Step.sol of OpenZeppelin and use the two-step ownership transfer pattern, in place of OpenZeppelin OwnableUpgradeable.