Papr contest - imare's results

Lines of code

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

Vulnerability details

Some NFTs collections can send airdops for their holders. One such collection are the MoonBirds. And send airdrops like this one.

When a users adds NFT as collateral with PaprController::addCollateral the PaprController contract becomes the new owner

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

PaprController contract is using onERC721Received callback function as a convenience/shortcut when a user deposits his/hers NFT to transfer/mint paper/sell the NFT in one go instead of calling additional functions (_addCollateral, increaseDebt, increaseDebtAndSell)

A possible airdrop will fail because the transaction will revert inside onERC721Received when decoding the IPaprController.OnERC721ReceivedArgs

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

Impact

Owners who transfer their NFTs to PaprController will lose a possible airdrop reward.

Proof of Concept

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

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

Tools Used

Manual review

Recommended Mitigation Steps

A possible solution would be to

inside onERC721Received function to decode data parameter only if there is something to decode (data.length != 0) so we don't risk to revert the transaction.

But we still have a problem that from parameter is the new owner (PaprController) and not the original owner who transferred the NFT. We have a problem how to bind the airdrop to the correct owner.

Additionally to all of the above such a collection should also be set as allowed which can only be done by the PaprController contract owner with setAllowedCollateral

Considering all of this there should be a simpler solution for such airdrop to be directly converted to PaprToken.. but again the TWAP/oracle price would need to exist for such action to go immediately trough.

Or even simpler solution would be to implement a recover functionality for the contract owner so the token doesn't get stuck inside the contract and when recovered decide how the NFT token will be distributed among rightful owners.