[M2] Incomplete reentrancy protection of submitAndDeposit()

[code423n4]

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()

function submitAndDeposit(address recipient) external payable returns (uint256 shares) {
    // Give the frxETH to this contract after it is generated
    _submit(address(this));
    // Approve frxETH to sfrxETH for staking
    frxETHToken.approve(address(sfrxETHToken), msg.value);
    // Deposit the frxETH and give the generated sfrxETH to the final recipient
    uint256 sfrxeth_recieved = sfrxETHToken.deposit(msg.value, recipient);  
    require(sfrxeth_recieved > 0, 'No sfrxETH was returned');
    return sfrxeth_recieved;
}

In this case if sfrxETHToken.deposit(msg.value, recipient); it could reenter depending on implementation (actually we only have access to interface IsfrxETH)

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

[FortisFortuna]

Was anything brickable or could funds be stolen? We saw this with Slither and deemed it a non-issue.

[itsmetechjay]

@peritoflores, can you chime in on the sponsor question above?

[peritoflores]

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

[0xean]

Downgrading to QA due to lack of more explicit POC. The possibility of re-entrancy alone is not enough for M