Trick or Treat
First Flight #27
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: low
Invalid

Redundant Owner Transfer Function

abhishekthakur

Summary

  • Root Cause: Implementation of changeOwner when transferOwnership is already available from OpenZeppelin's Ownable

  • Impact: Code bloat and potential confusion about which function to use

Vulnerability Details

https://github.com/Cyfrin/2024-10-trick-or-treat/blob/main/src/TrickOrTreat.sol#L156-L158

The contract implements a custom changeOwner function:

function changeOwner(address _newOwner) public onlyOwner {
transferOwnership(_newOwner);
}

This is redundant because:

  1. The contract inherits from OpenZeppelin's Ownable: contract SpookySwap is ERC721URIStorage, Ownable(msg.sender), ReentrancyGuard

  2. Ownable already provides a transferOwnership function with the same functionality

  3. The custom function adds no additional features or checks

  4. It merely wraps the existing transferOwnership function

POC

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
import "forge-std/Test.sol";
import "../src/SpookySwap.sol";
​
contract SpookySwapTest is Test {
SpookySwap spookySwap;
function setUp() public {
SpookySwap.Treat[] memory treats = new SpookySwap.Treat[]();
treats[0] = SpookySwap.Treat("Candy", 1 ether, "uri");
spookySwap = new SpookySwap(treats);
}
function testRedundantOwnershipTransfer() public {
address newOwner = address(0x123);
// Both functions do exactly the same thing
spookySwap.changeOwner(newOwner);
assertEq(spookySwap.owner(), newOwner);
address anotherOwner = address(0x456);
spookySwap.transferOwnership(anotherOwner);
assertEq(spookySwap.owner(), anotherOwner);
}
}

Impact

  • Code bloat

  • Potential confusion about which function should be used

  • Unnecessary gas costs for deployment

Tools Used

Manual Review

Recommendations

Remove the redundant changeOwner function and use OpenZeppelin's transferOwnership directly:

- function changeOwner(address _newOwner) public onlyOwner {
- transferOwnership(_newOwner);
- }
Updates

Appeal created

bube Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.