withdrawalFee, redeemFee, depositFee can be arbitrarily modified by the owner at any time without any user protections.

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0x837ffd8ef9213e85aecd0a9d41764f49015d96a5f7910756b43286fdea98045c
Severity: medium

Description:
Description
In the current design/implementation, the protocol fees can be changed at any time by the owner which can cause involuntary and unexpected loss of user's funds.

BaseMinter contract

@> 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);
    }

BaseMinterRedeem contract

@> uint256 public redeemFee = 0; // possible fee to cover bridging costs
    uint256 public constant MAX_REDEEM_FEE = 500; // max redeem fee 500bp (5%)

    event UpdateRedeemFee(uint256 _redeemFee);
    event Redeem(address indexed caller, address indexed receiver, uint256 amount);

    function previewRedeem(uint256 amount) public view virtual returns (uint256) {
        uint256 feeAmount = amount*redeemFee/FEE_DENOMINATOR;
        uint256 netAmount = amount-feeAmount;
        return netAmount;
    }

@>    function updateRedeemFee(uint256 newFee) public onlyOwner {
        require(newFee <= MAX_REDEEM_FEE, ">MaxFee");
        redeemFee = newFee;
        emit UpdateRedeemFee(newFee);
    }

BaseMinterWithdrawal contract

@> uint256 public withdrawalFee = 0; // possible fee to cover withdrawal costs
    uint256 public minWithdrawal = 1; // min withdrawal amount (wei)
    uint256 public constant MAX_WITHDRAWAL_FEE = 500; // max withdrawal fee 500bp (5%)
    uint256 public totalPendingWithdrawals = 0; // total pending withdrawals
    uint256 public totalUnclaimedWithdrawals = 0; // total unclaimed withdrawals
    uint256 public totalWithdrawalFees = 0; // total fees accumulated
    uint256 public nextWithdrawalId = 0; // counter for withdrawal IDs

<------- solidity code ------->

@> function updateWithdrawalFee(uint256 newFee) public onlyOwner {
        require(newFee <= MAX_WITHDRAWAL_FEE, ">MaxFee");
        withdrawalFee = newFee;
        emit UpdateWithdrawalFee(withdrawalFee);
    }

Attack Scenario

Attack scenario regarding increasing the depositFee:

depositFee = 0

1. Alice wants to deposit 100 ETH
2. Alice checks the net amount via previewDeposit()and the net amount that should be deposited is 100 ETH

function previewDeposit(uint256 amount) public view virtual returns (uint256) {
        uint256 feeAmount = amount*depositFee/FEE_DENOMINATOR;
        uint256 netAmount = amount-feeAmount;
        return netAmount;
    }

3. Meanwhile the owner decides to change the deposit fee to its maximum - 5% -depositFee = 500

uint256 public constant MAX_DEPOSIT_FEE = 500; // max deposit fee 500bp (5%)

4. Alice performs a deposit to the contract and instead of 100 expected staking tokens receives only 95 due to depositFee rise.

The same exploit is valid for:

• requestWithdrawal() in BaseMinterWithdrawal contract by increasing the withdrawalFee
• redeem() in NativeMinterRedeem and ERC20MinterRedeem contracts by increasing the redeemFee

Recommendations

The underlying issue is that users of the system can’t be sure what the behavior of a function call will be, and this is because the behavior can change at any time.

• Consider making the protocol fee rates a constants, ie, can not be changed;
• Implementing a logic that gives the user advance notice of changes with a time lock.
• Allow users to specify the exact fees they were expecting as a parameter

[0xRizwan]

Non-issue, Its upto protocol team to decide on fees and owner is trusted, moreover users are aware on deposit, withdraw, redeem fees.

[0xqaswed]

@0xRizwan can you please explain how exactly the users are protected from such fund losses via fee increases? I couldn't find such protection in the code. No matter that the owner is trusted, by changing the fee at any time can lead to unexpected behavior.
By the terms of the contest: "Issues that lead to an economic loss but do not lead to direct loss of on-chain assets" the submission should be valid.
Can't invalidate the issue just saying "The owner is trusted" and "The users are aware on deposit, withdraw, redeem fees."
How they be aware when the fees are changed?

[0xRizwan]

Charging fee to users on deposit/withdraw/redeem is not loss of funds but intended design of protocol. Protocol can only charge upto 5% max fee so it entirely depends on protocol to decide the fee to be charged. The root of this issue is admin being malicious which is out of scope under following rules:

Comments about the multi-sig owner being malicious (they are considered trusted and will be moved to the validator set)

Issues about the ability for multi-sig owner to set parameters in a way breaking the contract (they are trusted to be both non-malicious and non-stupid)

[0xqaswed]

@0xRizwan thank you for your comment,

The root cause of the issue is not the owner being malicious. The root issue is that the user can get charged a fee different from what they expect, and there's nothing in the code to prevent that - the attack scenario already described. Not a "good" feeling for a whale users to be charged with 5% instead 0%. I'm not sure this should be the "intended design of protocol".

[0xqaswed]

@0xRizwan please check and respond to the above comment. It's obvious this shouldn't be the "intended design of protocol".
By the terms of the contest: "Issues where the behavior of the contracts differs from the intended behavior (as described in the docs and by common sense), but no funds are at risk." this should be considered at least as low severity issue.