sherlock-audit / 2024-08-winnables-raffles-judging

6 stars 2 forks source link

Polite Pewter Goldfish - transferOwnership should be split into two separate functions #618

Closed sherlock-admin3 closed 3 months ago

sherlock-admin3 commented 3 months ago

Polite Pewter Goldfish

Low/Info

transferOwnership should be split into two separate functions

Summary

The transferOwnership function changes ownership of a contract in a single transaction. If an incorrect newOwner is provided, ownership may never be recovered.

Root Cause

The transferOwnership function changes ownership of a contract in a single transaction. If an incorrect newOwner is provided, ownership may never be recovered. A best practice is to split the ownership transfer into two functions: one for initiating the transfer and one for accepting the transfer. By splitting the functionality of transferOwnership into two functions— transferOwnership and acceptOwnership—the original owner will retain ownership until the new owner calls acceptOwnership. This will help prevent accidental transfer of ownership to an uncontrolled address.

function transferOwnership(address newOwner) external onlyOwner { owner = newOwner; emit OwnershipTransferred(msg.sender, newOwner); }

https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/main/public-contracts/contracts/WinnablesTicket.sol#L173

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

Impact

The vulnerability in the transferOwnership function can lead to a critical situation where ownership of the contract is irreversibly lost if an incorrect newOwner address is provided.

PoC

No response

Mitigation

use the described two-step process of transferOwnership and acceptOwnership to ensure an address is controllable before performing a full ownership transfer.