search

code-423n4 / 2022-06-notional-coop-findings

1 stars 1 forks source link

Re-entrancy in wfCashERC4626.withdraw() can lead to more gains in assets #68

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L187

Vulnerability details

This is a corrected version of the previous submission and typo mistakes corrected.

Impact

The withdraw() function in wfCashERC4626.sol can be re-entered at the point of _redeemInternal().

Assume asset tokens are sent to receiver after shares are burnt, and user re-enters withdraw() after _redeemInternal() is completed., (P.S: there's a separate issue on I submitted earlier on this part). But let's assume the function works as described by ERC4626 standard, User would have more assets each time shares are burnt.

Proof of Concept

  1. Assume _redeemInternal() works as normal and conversion of asset -> shares is 1:1
  2. Alice owns 5 ETH.
  3. Alice calls withdraw() with input 2 assets, her address for receiver and owner arguments.
  4. At _redeemInternal() , assume 2 fCash is burnt and 2 ETH is transferred to Alice
  5. Alice reenters withdraw() once _redeemInternal() is successful, Alice now owns 7 ETH
  6. Step 3 and 4 are repeated once more, Alice now owns 9 ETH.
  7. The function is re-entered until contract is drained of ETH.

Tools Used

Manual review

Recommended Mitigation Steps

Implement the check-effects-interactions pattern or nonReentrant Guards. It seems some account settlement within the vault needs to be done before the transfer of tokens to receiver as well.

ghost commented 2 years ago

The accounting checks are done here https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/contracts/wfCashLogic.sol#L212-L214

As long as owner has sufficient tokens and either msg.sender == owner or owner has given msg.sender sufficient allowance, you can continue to call this function .

jeffywu commented 2 years ago

Furthermore, there is a nonReentrant modifier on the _burn method (called by all redeem and withdraw functions) https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashLogic.sol#L207