search

code-423n4 / 2022-06-nested-findings

0 stars 1 forks source link

Missing reentrancy protections #15

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-nested/blob/main/contracts/operators/Beefy/BeefyVaultOperator.sol#L79-L91 https://github.com/code-423n4/2022-06-nested/blob/main/contracts/operators/Beefy/BeefyVaultOperator.sol#L36-L5091 https://github.com/code-423n4/2022-06-nested/blob/main/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L46-L59 https://github.com/code-423n4/2022-06-nested/blob/main/contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol#L91-L104 https://github.com/code-423n4/2022-06-nested/blob/main/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L46-L59 https://github.com/code-423n4/2022-06-nested/blob/main/contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol#L91-L104

Vulnerability details

Impact

The files below contain both deposit() and withdraw() functions which seem re-entrable at the point of calls that transfer tokens. The functions do not fully follow a checks-effects-interactions pattern, thus they can be re-entered multiple times. Depending on the contract and function, it may lead to loss of tokens or tokens theft.

Files affected: BeefyVaultOperator.sol BeefyZapBiswapLPVaultOperator.sol BeefyZapUniswapLPVaultOperator.sol

Proof of Concept

  1. In BeefyVaultOperator.sol, BeefyVaultOperator.withdraw() makes a vault.call(abi.encodeWithSignature("withdraw(uint256)", amount)) call
  2. withdraw() is reentered and more vault tokens are withdrawn.
  3. This continues until vault is drained.

Tools Used

Manual review

Recommended Mitigation Steps

Fully implement the checks-effects-interactions pattern. Applying the nonReentrant modifier to the external functions may be recommended.

obatirou commented 2 years ago

Missing reentrancy protections (disputed)

If we call withdraw from a EOA/contract (using call), we are leaving the NestedFactory context since it's a delegatecall. You can't perform a reentrancy attack in the NestedFactory context to withdraw multiple times. The NestedFactory functions already have the reentrancy protection.