code-423n4 / 2022-11-paraspace-findings

7 stars 4 forks source link

Operator can grief PoolMarketplace #435

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/misc/marketplaces/X2Y2Adapter.sol#L31

Vulnerability details

Impact

An operator can grief PoolMarketplace by draining its WETH into X2Y2

In its run function X2Y2 checks if input.shared.amountToEth is greater than 0. If it is, WETH is moved from msg.sender to X2Y2 with amountToEth is set to the amount of WETH unwrapped. amountToEth is the result of amountToEth minus the uint256 returned by the _run function. The X2Y2 adapter doesn’t check input.shared.amountToEth.

What is the currency set to? DAI, in this case

Why? When the _run function is called it returns an amount which is subtracted from and saved to amountToEth.

Why does this matter? When amountToEth is 0 the X2Y2 contract will not send any ETH back.

What does this mean? When an operator uses calls the X2Y2 contract via PoolMarketplace setting his currency to an accepted ERC20, DAI, and input.shared.amountToEth to a non zero amount, WETH is moved from PoolMarketplace to the X2Y2 contract in the specified amount because PoolMarketplace is the msg.sender. Because currency is set to DAI, in this case, it is moved from PoolMarketplace to X2Y2 and saved to the nativeAmount which is used to determine amountToEth.

Succinctly? This bug allows an operator to grief the PoolMarketplace contract. It does this by setting input.shared.amountToEth to the amount of WETH in the contract, setting currency to an accepted ERC20 that isn’t WETH and setting amount to equal PoolMarketplaces’ total WETH amount.

Proof of Concept

PoolMarketplace has 50 WETH Operator creates NFT Operator sets NFTs price to 50 DAI Operator calls buyWithCredit using X2Y2 adapter with currency set to DAI and with input.shared.amountToEth = 50 WETH When the run function within X2Y2 is called 50 WETH is pulled from PoolMarketplace into X2Y2 setting amountEth to 50. amountEth is set to 0 via the _takePayment function which transfers the 50 DAI to the X2Y2 setting nativeAmount to 50. When the _run function returns it returns 50, 50 - 50, setting amountEth to 0. 50 DAI is paid to the operator less fees PoolMarketplace isn’t refunded it’s ETH because amountEth == 0.

Recommended Mitigation Steps

Require input.shared.amountToEth && input.shared.amountToWeth == 0.

c4-judge commented 1 year ago

dmvt marked the issue as duplicate of #54

c4-judge commented 1 year ago

dmvt marked the issue as satisfactory

c4-judge commented 1 year ago

dmvt marked the issue as not a duplicate

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

dmvt marked the issue as grade-b

dmvt commented 1 year ago

There's really no reason for anyone to ever do this.