Open hats-bug-reporter[bot] opened 2 months ago
stZETA
is OOS
Hey @0xRizwan, thanks for the quick reply.
I just gave an example with stZETA
, but the issue is that stZETA
has pause/unpause
with onlyOwner
and since Minters will be the owners of the staked tokens pause/unpause
won't be able to be invoked since in both NativeMinter
and ERC20Minter
it doesn't have functions which call it like this:
abstract contract BaseMinter is Ownable, ReentrancyGuard {
uint256 public depositFee = 0; // possible fee to cover bridging costs
uint256 public constant MAX_DEPOSIT_FEE = 500; // max deposit fee 500bp (5%)
uint256 public constant FEE_DENOMINATOR = 10000; // fee denominator for basis points
// Staking token
IERC20 public stakingToken;
constructor(address _stakingToken) {
stakingToken = IERC20(_stakingToken);
}
event UpdateDepositFee(uint256 _depositFee);
event TransferStakingTokenOwnership(address indexed _newOwner);
event Mint(address indexed caller, address indexed receiver, uint256 amount);
function previewDeposit(uint256 amount) public view virtual returns (uint256) {
uint256 feeAmount = amount*depositFee/FEE_DENOMINATOR;
uint256 netAmount = amount-feeAmount;
return netAmount;
}
function updateDepositFee(uint256 newFee) public onlyOwner {
require(newFee <= MAX_DEPOSIT_FEE, ">MaxFee");
depositFee = newFee;
emit UpdateDepositFee(newFee);
}
function transferStakingTokenOwnership(address newOwner) public onlyOwner {
stakingToken.transferOwnership(newOwner);
emit TransferStakingTokenOwnership(newOwner);
}
function mint(uint256 amount, address receiver) public onlyOwner {
stakingToken.mint(receiver, amount);
emit Mint(address(msg.sender), receiver, amount);
}
+ function pause() public onlyOwner {
+ stakingToken.pause();
+ }
+ function unpause() public onlyOwner {
+ stakingToken.unpause();
+ }
}
I also specified it here - The pause functions in all staked tokens (/tokens) is not possible to be invoked because Minter contracts are designed to hold the staked tokens ownership and there is no exposed function that calls stX.pause/unpause in Minter contracts.
The fix that I gave is for the tokens itself and it is for sure better, but the actual issue is inside the Minter.sol and if the tokens are meant not to be changed this can also be fixed from the Minters where the problem is.
@Slavchew Correct. In context of this competition scope, the pause and unpause of stROSE
won't be able to call by contract owner which is supposed to be stROSEMinter contract. This would break the functionality of protocol and should be Medium severity under following contest rule:
Medium Attacks that make essential functionality of the contracts temporarily unusable or inaccessible.
It was made intentionally – multisig owner cannot pause/unpause stX transfers (while stToken is ERC20, ERC20Burnable, Pausable, Ownable
). No issues with this.
How no issue, what is the idea to inherit Pausable to expose public functions for that action, to restrict the beforeTokenTransfer and then to tell that it is intended to not be used. This wasn't specified in the readme or the details of the competition and it is also ridiculous to tell this is intended as the tokens do not have pausing and the code assume they have.
Since sponsor confirmed its intended design to not call pause function via multisig so its non-issue.
I have also second thought on pausing of stROSE
tokens if its needed. The minter contract owner can change anytime to transfer ownership of stROSE to other address and the pause functions from stROSE
can be directly accessed in such case. This won't break the whole contract functionality but it would make few more steps of work for contract owner. Therefore, i agree with sponsor on this issue so marking it invalid.
Yeah, that's true, but in case of emergency, instead of instantly pause the contract and no user funds to be able to be drain, the owners should perform multisig call to change the owner of the stRose token which will need first to meet some threshold then to pause it.
It is borderline between Medium/Low but it is not invalid at all.
owners should perform multisig call to change the owner
The same owners should perform multisig call to pause the contract, in case this function is implemented.
This wasn't specified in the readme or the details of the competition and it is also ridiculous to tell this is intended as the tokens do not have pausing and the code assume they have.
stToken.sol (which is not in scope) is OpenZeppelin standard ERC20 – with ownership, pause & burn. However, it does not mean all of these functions should be used in general (base) minters. By default, minter cannot pause stToken txs – its intended design for general minters.
Custom minters can have such function (e.g. if it's security requirement from specific protocol in order to integrate with them), thus stToken.sol allows to pause, but by default and in default Minter.sol it's not used.
We keep this issue as invalid.
Github username: -- Twitter username: -- Submission hash (on-chain): 0x8403d03d97414cc2c22177914b4a63f3403f711c9d321cba93be0b092f6ad09a Severity: high
Description: Description\ The pause functions in all staked tokens (
/tokens
) is not possible to be invoked because Minter contracts are designed to hold the staked tokens ownership and there is no exposed function that callsstX.pause/unpause
in Minter contracts.Attack Scenario\ Describe how the vulnerability can be exploited.
Recommended Mitigation
This can be fixed by removing
onlyOwner
from thepause/unpause
functions and restrict the function to a role (PAUSER_ROLE
) that can be added to multiple addresses and have the pause logic still usable.