Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential reentrancy in collectWithdrawalFees() function due to violation of CEI #26

Open
hats-bug-reporter bot opened this issue Sep 2, 2024 · 1 comment
Labels
bug Something isn't working Low - Lead auditor Low

Comments

@hats-bug-reporter
Copy link

Github username: --
Twitter username: --
Submission hash (on-chain): 0x5529151e09b33810031083f111e067ffb0d1a1e92844738f22635f16497c35a2
Severity: low

Description:
Description
In Minter.sol, an owner can call collectWithdrawalFees() to withdraw the fees. This function has used safeTransfer which transfers the fees to receiver address.

    function collectWithdrawalFees(address receiver) public onlyOwner {
        require(totalWithdrawalFees > 0, "ZeroFees");
        stakingToken.safeTransfer(receiver, totalWithdrawalFees);
@>        totalWithdrawalFees = 0;
        emit CollectWithdrawalFees(address(msg.sender), receiver, totalWithdrawalFees);
    }

The issue here is that, collectWithdrawalFees() could allow reentrancy issue due to violation of Checks, Effects, Interaction pattern. where the effects are happening after transferring the tokens. It must be noted that, all external function calls must be performed at the end of functions and state should be updated before to it.

Recommendations
Follow CEI pattern or add nonReentranct modifier on collectWithdrawalFees() to avoid reentrancy issues.

    function collectWithdrawalFees(address receiver) public onlyOwner {
        require(totalWithdrawalFees > 0, "ZeroFees");
+        totalWithdrawalFees = 0;
        stakingToken.safeTransfer(receiver, totalWithdrawalFees);
-        totalWithdrawalFees = 0;
        emit CollectWithdrawalFees(address(msg.sender), receiver, totalWithdrawalFees);
    }
@hats-bug-reporter hats-bug-reporter bot added the bug Something isn't working label Sep 2, 2024
@ilzheev ilzheev added the Low label Sep 3, 2024
@ilzheev
Copy link

ilzheev commented Sep 9, 2024

@0xRizwan AccumulatedFinance/contracts-v2@73a5729

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Low - Lead auditor Low
Projects
None yet
Development

No branches or pull requests

2 participants