Biconomy: Nexus
Biconomy
HardhatFoundry
30,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Use of 'Ownable' Instead of 'Ownable2Step'

juggernaut63

File location:

https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/common/Stakeable.sol#L27

Summary

'Stakeable' contracts use 'Ownable' which allows direct transfer of ownership without confirmation from the new owner. This may result in risks if the address entered is incorrect or cannot handle the ownership.

Vulnerability Details

In a 'Stakeable' contract, ownership can be immediately transferred to a new address using the '_setOwner' function of 'Ownable'. However, if the address entered is incorrect or it is not possible to manage ownership (for example, due to a typo or incorrect address), then the contract will be in an unmanageable state. This may result in loss of control over the contract.

Impact

If the new address provided is incorrect or cannot handle ownership, the 'Stakeable' contract cannot be managed any more. This may result in loss of staked funds and loss of control over important contract functions, such as withdrawal or locking of funds.

Tools Used

  • Inspection manual

  • Solidity

Recommendations

Code snippet:

contract Stakeable is Ownable, IStakeable {
/// @notice Error thrown when an invalid EntryPoint address is provided.
error InvalidEntryPointAddress();
​
constructor(address newOwner) {
_setOwner(newOwner);
}
​
/// @notice Stakes a certain amount of Ether on an EntryPoint.
/// @dev The contract should have enough Ether to cover the stake.
/// @param epAddress The address of the EntryPoint where the stake is added.
/// @param unstakeDelaySec The delay in seconds before the stake can be unlocked.
function addStake(address epAddress, uint32 unstakeDelaySec) external payable onlyOwner {
require(epAddress != address(0), InvalidEntryPointAddress());
IEntryPoint(epAddress).addStake{ value: msg.value }(unstakeDelaySec);
}
​
/// @notice Unlocks the stake on an EntryPoint.
/// @dev This starts the unstaking delay after which funds can be withdrawn.
/// @param epAddress The address of the EntryPoint from which the stake is to be unlocked.
function unlockStake(address epAddress) external onlyOwner {
require(epAddress != address(0), InvalidEntryPointAddress());
IEntryPoint(epAddress).unlockStake();
}
​
/// @notice Withdraws the stake from an EntryPoint to a specified address.
/// @dev This can only be done after the unstaking delay has passed since the unlock.
/// @param epAddress The address of the EntryPoint where the stake is withdrawn from.
/// @param withdrawAddress The address to receive the withdrawn stake.
function withdrawStake(address epAddress, address payable withdrawAddress) external onlyOwner {
require(epAddress != address(0), InvalidEntryPointAddress());
IEntryPoint(epAddress).withdrawStake(withdrawAddress);
}
}

Fixed:

  • Replace ‘import "@openzeppelin/contracts/access/Ownable.sol";’ with ‘import "@openzeppelin/contracts/access/Ownable2Step.sol";’.

  • Replace ‘contract Stakeable is Ownable, IStakeable’ with ‘contract Stakeable is Ownable2Step, IStakeable’.

  • Replace ‘_setOwner(newOwner);’ dengan ‘_transferOwnership(newOwner);’ on constructor.

Updates

Lead Judging Commences

0xnevi Lead Judge
11 months ago
0xnevi Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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