amshirif
commented
7 months ago I do not understand what the person is trying to say. It seems as though they are saying that _msgSender() will not work and that anyone would be able to call the onlyOwner call. This is not accurate. If you change the unit tests to any other address besides the deployer it fails.
sherlock-admin2
Sabit
high
Anyone can call the createStakingRewards function because of wrong "onlyOwner" implementation
Summary
Anyone can call the createStakingRewards function because of wrong "onlyOwner" implementation
Vulnerability Detail
Checking the package.json of the contract, it shows that the contract uses Openzeppelin's contract version 5 upward:
"dependencies": { "@openzeppelin/contracts": "^5.0.1", "@openzeppelin/contracts-upgradeable": "^5.0.1" }
The StakingRewardsFactory contract imported the Ownable.sol contract, consequently, which is version 5. And Ownable.sol inturn imported Context.sol. Here's the link to Ownable.sol:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol
Now let's look at the key functions in Ownable.sol relevant here:
` constructor(address initialOwner) { if (initialOwner == address(0)) { revert OwnableInvalidOwner(address(0)); } _transferOwnership(initialOwner); }
modifier onlyOwner() { checkOwner(); ; }
function owner() public view virtual returns (address) { return _owner; }
function _checkOwner() internal view virtual { if (owner() != _msgSender()) { revert OwnableUnauthorizedAccount(_msgSender()); } } function _transferOwnership(address newOwner) internal virtual { address oldOwner = _owner; _owner = newOwner; emit OwnershipTransferred(oldOwner, newOwner); }`
Here's the key functioon in Context.sol:
function _msgSender() internal view virtual returns (address) { return msg.sender; }
In the StakingRewardsFactory contract, _msgSender() is passed into Ownable and initialized as the owner. However, the vulnerability in using _msgSender() as the owner is that _msgSender() is a function that only returns msg.sener. That is, the immediate caller.
What this means is that onlyOwner used in createStakingRewards function will not revert when anyone calls the function. The onlyOwner function as in Ownable.sol returns the initialOwner set in the Ownable.sol constructor. In StakingRewardsFactory, initialOwner in this sense is set to _msgSender() - Ownable(_msgSender()) which calls a function (_msgSender()) in the real sense.
Note that _msgSender() is a function in Context.sol cited above that merely returns the immediate sender of a transaction.
The above vulnerability comes from the fact that the contract uses the latest version of Openzeppeliln contracts (version 5 and above) wrongly.
Had it been that the contract uses a version lesser than 5 of Openzeppelin contracts such as v.4.9.5 or lower, this wouldn't have been a vulnerability.
Here's how Ownable.sol looks like in v.4.9.5:
` constructor() { _transferOwnership(_msgSender()); }
function owner() public view virtual returns (address) { return _owner; }
function _checkOwner() internal view virtual { require(owner() == _msgSender(), "Ownable: caller is not the owner"); }
function _transferOwnership(address newOwner) internal virtual { address oldOwner = _owner; _owner = newOwner; emit OwnershipTransferred(oldOwner, newOwner); }`
See: https://github.com/OpenZeppelin/openzeppelin-contracts/releases/tag/v4.9.5
From the above, ownership was transferred directly to _msgSender() in the constructor of Ownable.sol. This makes the sender of the contract the owner of the contract.
Impact
Anyone can call the createStakingRewards function
Code Snippet
https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/telx/core/StakingRewardsFactory.sol#L43-L47
https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/telx/core/StakingRewardsFactory.sol#L32-L32
Tool used
Manual Review
Recommendation
msg.sender should be used instead of _msgSender() in the constructor
Duplicate of #136