search

code-423n4 / 2021-11-nested-findings

1 stars 1 forks source link

NestedFactory inherits from OpenZeppelin Multicall, which multicall function allows reusage of msg.value within ETH depositing #227

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

hyh

Vulnerability details

Impact

Contract holdings can be emptied as malicious user will do deposit/withdraw to extract value. This is possible because after _transferInputTokens system uses contract balance for user's operations, assuming that equivalent value was transferred before.

Proof of Concept

NestedFactory inherits from Multicall, which multicall(bytes[] calldata data) function allows the reusage of msg.value, which will persist over deposit calls.

https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L20

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Multicall.sol

From https://forum.openzeppelin.com/t/multicall-by-oz-and-makerdao-has-a-difference/9350/2:

OpenZeppelin’s is a module that a developer can include in their own contract to allow users to batch multiple function calls to it in one transaction. Since msg.sender is preserved, it’s equivalent to sending multiple transactions from an EOA (non-contract account).
When a function is executed with delegatecall these values do not change:
address(this)
msg.sender
msg.value

Recommended Mitigation Steps

Treat ETH exclusively, not allowing ETH operations to be batched at all.

maximebrugel commented 2 years ago

Duplicated : #226

alcueca commented 2 years ago

Identical to #226, by the same warden.