Incorrect usage of safeTransferFrom traps fees in Papr Controller

[code423n4]

Lines of code

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L382-L384

Vulnerability details

Impact

Because the Papr Controller never gives approval for ERC20 transfers, calls to safeTransferFrom on the Papr token will revert with insufficient approval. This will trap proceeds from auctions in the contract and prevent the owner/ DAO from collecting fees, motivating the rating of high severity. The root cause of this issue is misusing safeTransferFrom to transfer tokens directly out of the contract instead of using transfer directly. The contract will hold the token balance and thus does not need approval to transfer tokens, nor can it approve token transfers in the current implementation.

Proof of Concept

Comment out this token approval as the controller contract does not implement functionality to call approve. It doesn't make sense to "prank" a contract account in this context because it deviates from the runtime behavior of the deployed contract. That is, it's impossible for the Papr Controller to approve token transfers. Run forge test -m testSendPaprFromAuctionFeesWorksIfOwner and observe that it fails because of insufficient approvals. Replace the call to safeTransferFrom with a call to transfer(to, amount) and rerun the test. It will now pass and correctly achieve the intended behavior.

Tools Used

Foundry

Recommended Mitigation Steps

Call transfer(to, amount) instead of safeTrasferFrom here. Note, it's unnecessary to use safeTransfer as the Papr token doesn't behave irregularly.

[c4-judge]

trust1995 marked the issue as satisfactory

[Jeiwan]

Good finding! In the current implementation PaprController doesn't accumulate fees, so it may not cause a loss of funds.

[c4-sponsor]

wilsoncusack marked the issue as sponsor confirmed

[trust1995]

@wilsoncusack , will you agree that in the current iteration of the code, we can consider this a M level find as no funds are at risk?

[c4-judge]

trust1995 marked the issue as selected for report

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)

[wilsoncusack]

@trust1995 it's a tough call. No funds are at risk because we burn fees. So these functions are not needed or used right now. But if we did not burn fees then all papr fees would be stuck. In the whitepaper we mention the idea of an insurance fund. Tempted to say high?

[trust1995]

I have reviewed this finding along with several other judges, and believe it is ultimately of Med severity. Thank you for your input.

[C4-Staff]

JeeberC4 marked the issue as primary issue