Possible loss of ownership of the vault, due to the use of ownable.sol for contract ownership.

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L8

Vulnerability details

Impact

possible loss of ownership of contracts like the vault.

Proof of Concept

The ownable.sol contract is used to set and also transfer ownership of contracts between accounts, but this contract has a shortcoming as ownership can be accidentally transfered to the wrong address due to to typographical error when inputing the address, or mistakenly transfered to address(0). this shortcoming is not present in the ownable2steps.sol as transfer of ownership is done in two steps where the new owner has to accept ownership for ownership to be transfered to new owner. this particular issue can affect the deployvault() function in Vaultfactory.sol as there is check against the address zero, but this does not ensure that there cant be typos, when inputing the adddress of the owner.

Tools Used

Mnual review

Recommended Mitigation Steps

Consider using the ownable2steps contract to ensure that ownership is not lost.

Assessed type

Access Control

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[PierrickGT]

We use our own library that implements two steps ownership: https://github.com/GenerationSoftware/pt-v5-vault/blob/0b91c3dcaecbcc0a786297a00f46d96109760202/src/Vault.sol#L8-L9 https://github.com/GenerationSoftware/pt-v5-vault/blob/ce55b0948c3630b5d1c069fe948ce0a8aa58b229/.gitmodules#L14 https://github.com/pooltogether/owner-manager-contracts/blob/3eba990436e82ace733cf00f595ae05674f88855/contracts/Ownable.sol#L92

This report is invalid.