code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L109-L114 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccountNoAuth.sol#L109-L114

Vulnerability details

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.

Proof of Concept

As it is shown in the below code, the single-step ownership transfer pattern is used. This can prompt all the onlyOwner methods noncallable.

function setOwner(address _newOwner) external mixedAuth {
    require(_newOwner != address(0), "Smart Account:: new Signatory address cannot be zero");
    address oldOwner = owner;
    owner = _newOwner;
    emit EOAChanged(address(this), oldOwner, _newOwner);
}

Tools Used

Manual Review, VSCode

Recommended Mitigation Steps

Use Ownable2Step of OpenZeppelin and use the two-step ownership transfer pattern.

c4-judge commented 1 year ago

Duplicate of https://github.com/code-423n4/2023-01-biconomy-findings/issues/423

c4-sponsor commented 1 year ago

livingrockrises marked the issue as sponsor confirmed