First instance:

Summary

The contract OwnershipFacet lacks a check for the zero address in its transferOwnership function, allowing _newOwner to be set to address(0). This oversight can lead to unexpected behavior or contract lock-up scenarios.

Vulnerability Detail

The vulnerability arises from the omission of a check to ensure _newOwner is not the zero address (address(0)) before assigning it to s.sys.ownerCandidate in the transferOwnership function.

Impact

• Contract Lock: If _newOwner is set to the zero address, no subsequent candidate can claim ownership because the check in claimOwnership will fail since s.sys.ownerCandidate is address(0).:

• Unexpected Behavior: Allowing any address to be set as _newOwner, including the zero address, can lead to unintended consequences or exploits.

Tools Used

Manual Code Review

Recommendations

Add Check for Zero Address: Ensure _newOwner is not address(0) before assigning it to s.sys.ownerCandidate in the transferOwnership function.

Second Instance:

Summary

The vulnerability identified in the LibDiamond library pertains to the ownership transfer logic, specifically within the setContractOwner function. This flaw allows the contract owner to be set to the zero address, rendering the contract unusable and locking out the ability to perform critical operations that typically require owner authorization.

Vulnerability Details

The issue arises in the setContractOwner function, which is designed to change the ownership of the contract to a new address. However, the function lacks a critical check to ensure that the new owner address is not the zero address (address(0)). This oversight means that the contract owner can be inadvertently set to the zero address, which would effectively disable the contract, as most operations would require the owner's approval or consent, which cannot be obtained from the zero address.
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/4e0ad0b964f74a1b4880114f4dd5b339bc69cd3e/protocol/contracts/libraries/LibDiamond.sol#L52-L58

Impact

The primary impact of this vulnerability is the loss of control over the contract. Setting the contract owner to the zero address would make the contract unusable, as the owner's permissions are required for critical operations such as upgrading the contract, changing its configuration, or even transferring ownership again.

Tools Used

Manual Code Review

Recommended Mitigation

A simple require statement should be added to the setContractOwner function to ensure that the new owner address is not the zero address