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

Bent Concrete Crocodile - {actor} will {impact} {affected party} #255

Open
sherlock-admin4 opened this issue Dec 31, 2024 · 0 comments
Open

Comments

@sherlock-admin4
Copy link

Bent Concrete Crocodile

High

{actor} will {impact} {affected party}

Summary

1.	The sell() function in the contract uses the nonReentrant modifier, which is intended to prevent reentrancy issues caused by external repeated calls to the function.
2.	However, in the liquidateNumaBorrower() function, the contract indirectly calls its own sell() function through an external call using NumaVault(address(this)).sell(...).
3.	OpenZeppelin’s ReentrancyGuard determines “reentrancy” within the same call context. External calls (e.g., this -> external -> this) are treated as a new call context, which causes the nonReentrant modifier to either fail or behave abnormally.
4.	If sell() or its subsequent processes (such as sending fees to fee_address via a .call()) trigger callbacks to other contract functions, it can result in sensitive logic being executed multiple times before all state updates are finalized, thus enabling a reentrancy attack.

Root Cause

A key red flag in this contract is that liquidateNumaBorrower() makes an external call back into itself—specifically:

// Inside liquidateNumaBorrower(...)
uint lstReceived = NumaVault(address(this)).sell(
receivedNuma,
lstAmount,
address(this)
);

Even though sell() is marked nonReentrant, calling it externally via NumaVault(address(this)) re‑enters the same contract in a new call context. In other words, the nonReentrant guard in OpenZeppelin’s ReentrancyGuard only protects calls within the same execution context. As soon as you do an external call back into the same contract, the reentrancy lock can be bypassed or behave unpredictably.

Why This is a Problem
• nonReentrant is context-scoped. If function A in a contract is not nonReentrant but it calls an external function B in the same contract, B’s nonReentrant check sees a “fresh” call context and will happily run—even though we are conceptually still “inside” the original contract execution.
• Potential for reentrancy. Depending on how the rest of the contract or fallback functions are implemented, this can open up reentrancy-like scenarios, because an attacker may exploit this external call path (especially if any state is updated between these calls).

Typical Patterns to Avoid This
1. Use internal function calls instead of external calls for the parts that need to be single-transaction atomic. If liquidateNumaBorrower() just wants to “do a sell,” it should call an internal helper function (not an external call to sell in the same contract).
2. Split up the logic that needs to be nonReentrant so that you do not rely on external calls into your own contract.

Because the contract uses nonReentrant on buy() and sell() but then calls sell() via an external call to its own address, this can break the intended reentrancy protection pattern. That is the most critical logic flaw to fix in this code.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

No response

PoC

No response

Mitigation

No response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant