hats-finance / OLD-Accumulated-finance-0x75278bcc0fa7c9e3af98654bce195eaf3bb6a784

MIT License
0 stars 0 forks source link

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

Open hats-bug-reporter[bot] opened 2 months ago

hats-bug-reporter[bot] commented 2 months ago

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);
    }
ilzheev commented 2 months ago

@0xRizwan https://github.com/AccumulatedFinance/contracts-v2/commit/73a5729f714fe01118849cad930ec071c84bdfe6