Open code423n4 opened 2 years ago
Was anything brickable or could funds be stolen? We saw this with Slither and deemed it a non-issue.
@peritoflores, can you chime in on the sponsor question above?
Hi @FortisFortuna ,
I reported this issue as "medium" because according to code4rena standard.
"Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements."
The main problem is that sfrxETHToken is just an interface so I do not have the code to say that is where the "hypotethical" is. In any case it is pretty general that all the functions in most of the protocols that are related to "deposit" or "withdraw" are always protected by non-reentrant modifier, unless you have no any external call which is not your case. @itsmetechjay
Downgrading to QA due to lack of more explicit POC. The possibility of re-entrancy alone is not enough for M
Lines of code
https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L70
Vulnerability details
Impact
Risk of reentrancy in
submitAndDeposit
function.PoC
I see that you added the non reentrant modifier to the internal function
_submit()
.The problem with this is that you are not protecting some parts of the function
submitAndDeposit()
In this case if
sfrxETHToken.deposit(msg.value, recipient);
it could reenter depending on implementation (actually we only have access to interfaceIsfrxETH
)In any case it is better to add reentrant modifier to every external function that you want to protect.
Recommended
Remove
nonReentrant
modifier from_submit()
and add it to every function that uses it