Beatland Festival
First Flight #44
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: medium
Invalid

[H-1] Owner renounce can set BeatToken contract to address(0)

emerjux

Root + Impact

Description

Because BeatToken is also Ownable2Step and Ownable from the OpenZeppelin contracts, any owner can call the function renounceOwnership() and set the contract owner's address to address(0).

The function renounceOwnership() is inherited via openzeppelin-contracts/contracts/access/Ownable.sol it is true that it can only be executed by the owner at the moment, but if someone executes it, then it will render the BeatToken unavailable.

function renounceOwnership() public virtual onlyOwner {
_transferOwnership(address(0));
}

Risk

Likelihood:

  • The current owner of the BeatToken is disgrunted and decides to renounce on the ownership

  • The owner private keys are compromised and the attacker issues the renounceOwnership() function to do a DoS

Impact:

  • After the execution of renounceOwnership() it is not possible to transfer to a new owner

  • After the execution of renounceOwnership() it is not possible to set any festival contract

  • Seems when it happens no operations required by the owner are possible, including withdraw.

Proof of Concept

Add the entry beatToken.renounceOwnership(); at any point of any test, even just after the creation of the token

function setUp() public {
owner = address(this); // Test contract is the owner
festivalContract = makeAddr("festivalContract");
user = makeAddr("user");
emerjux = makeAddr("emerjux");
beatToken = new BeatToken();
beatToken.renounceOwnership(); // Just after instantiating the token
}
function test_SetFestivalContract_AfterOwnershipTransfer_andReject() public {
address newOwner = makeAddr("newOwner");
beatToken.renounceOwnership(); // or during the test at any point
// Transfer ownership fails as the owner has renounced
// with the fixed code, this will not revert
vm.expectRevert();
beatToken.transferOwnership(newOwner);
// New owner accepts ownership after renounce of previous owner also fails
vm.prank(newOwner);
beatToken.acceptOwnership();
​
// New owner can't set it as he has previously renounced ownership
vm.prank(newOwner);
vm.expectRevert();
beatToken.setFestivalContract(festivalContract);
​
// with the fixed code, this will not revert
// This is because the ownership has been renounced, and the contract has no owner now
vm.prank(owner);
vm.expectRevert();
beatToken.setFestivalContract(festivalContract);
​
}

Recommended Mitigation

In src/BeatToken.sol add the following function, I recommend to add it at the end of the code for ease of read.

+ function renounceOwnership() public virtual override onlyOwner {
+ _transferOwnership(owner());
+ }
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 month ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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