ParaSpace contest - 0x52's results

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/ui/WPunkGateway.sol#L77-L95 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/ui/WPunkGateway.sol#L129-L155 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/ui/WPunkGateway.sol#L167-L193

Vulnerability details

Impact

All CryptoPunk deposits can be stolen

Proof of Concept

CryptoPunks were created before the ERC721 standard. A consequence of this is that they do not possess the transferFrom method. To approximate this a user can offerPunkForSaleToAddress for a price of 0 to effectively approve the contract to transferFrom.

WPunkGateway.sol#L77-L95

function supplyPunk(
    DataTypes.ERC721SupplyParams[] calldata punkIndexes,
    address onBehalfOf,
    uint16 referralCode
) external nonReentrant {
    for (uint256 i = 0; i < punkIndexes.length; i++) {
        Punk.buyPunk(punkIndexes[i].tokenId);
        Punk.transferPunk(proxy, punkIndexes[i].tokenId);
        // gatewayProxy is the sender of this function, not the original gateway
        WPunk.mint(punkIndexes[i].tokenId);
    }
    Pool.supplyERC721(
        address(WPunk),
        punkIndexes,
        onBehalfOf,
        referralCode
    );
}

The current implementation of WPunkGateway#supplyPunk allows anyone to execute and determine where the nTokens are minted to. To complete the flow supply flow a user would need to offerPunkForSaleToAddress for a price of 0 to WPunkGateway. After they have done this, anyone can call the function to deposit the punk and mint the nTokens to themselves, effectively stealing it.

Example: User A owns tokenID of 1. They want to deposit it so they call offerPunkForSaleToAddress with an amount of 0, effectively approving the WPunkGateway to transfer their CryptoPunk. User B monitors the transactions and immediately calls supplyPunk with themselves as onBehalfOf. This completes the transfer of the CryptoPunk and deposits it into the pool but mints the nTokens to User B, allowing them to effectively steal the CryptoPunk

The same fundamental issue exists with acceptBidWithCredit and batchAcceptBidWithCredit

Tools Used

Manual Review

Recommended Mitigation Steps

Query the punkIndexToAddress to find the owner and only allow owner to deposit:

    for (uint256 i = 0; i < punkIndexes.length; i++) {
+       address owner = Punk.punkIndexToAddress(punkIndexes[i].tokenId);
+       require(owner == msg.sender);

        Punk.buyPunk(punkIndexes[i].tokenId);
        Punk.transferPunk(proxy, punkIndexes[i].tokenId);
        // gatewayProxy is the sender of this function, not the original gateway
        WPunk.mint(punkIndexes[i].tokenId);
    }

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/SupplyLogic.sol#L462-L512

Vulnerability details

Impact

Adversary can DOS user and make them pay more gas by sending them collateral

Proof of Concept

if (fromConfig.isUsingAsCollateral(reserveId)) {
    if (fromConfig.isBorrowingAny()) {
        ValidationLogic.validateHFAndLtvERC20(
            reservesData,
            reservesList,
            usersConfig[params.from],
            params.asset,
            params.from,
            params.reservesCount,
            params.oracle
        );
    }

    if (params.balanceFromBefore == params.amount) {
        fromConfig.setUsingAsCollateral(reserveId, false);
        emit ReserveUsedAsCollateralDisabled(
            params.asset,
            params.from
        );
    }

    //@audit collateral is automatically turned on for receiver
    if (params.balanceToBefore == 0) {
        DataTypes.UserConfigurationMap
            storage toConfig = usersConfig[params.to];

        toConfig.setUsingAsCollateral(reserveId, true);
        emit ReserveUsedAsCollateralEnabled(
            params.asset,
            params.to
        );
    }
}

The above lines are executed when a user transfer collateral to another user. If the sending user currently has the collateral enabled and the receiving user doesn't have a balance already, the collateral will automatically be enabled for the receiver. Since the collateral is enabled, it will now be factored into the health check calculation. This increases gas for the receiver every time the user does anything that requires a health check (which ironically includes turning off a collateral). If enough different kinds of collateral are added to the platform it may even be enough gas to DOS the users.

Tools Used

Manual Review

Recommended Mitigation Steps

Don't automatically enable the collateral for the receiver.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/ParaSpaceOracle.sol#L138-L153

Vulnerability details

Impact

Outdated or incorrect asset prices

Proof of Concept

Chainlink has officially deprecated the latestAnswer method, recommending to use latestRoundData instead. This offers much better data validation to ensure that the price data is current and correct.

Tools Used

Manual Review

Recommended Mitigation Steps

Use latestRoundData instead. Validate outputs and pass to the fallback oracle if data is stale or incorrect.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/ValidationLogic.sol#L414-L440

Vulnerability details

Impact

Liquidation of assets when account isn't actually under-collateralized

Proof of Concept

function validateSetUseERC20AsCollateral(
    DataTypes.ReserveCache memory reserveCache,
    uint256 userBalance
) internal pure {
    require(userBalance != 0, Errors.UNDERLYING_BALANCE_ZERO);

    IXTokenType xToken = IXTokenType(reserveCache.xTokenAddress);

    //@audit user is unable to change status of sApe manually
    require(
        xToken.getXTokenType() != XTokenType.PTokenSApe,
        Errors.SAPE_NOT_ALLOWED
    );

    (
        bool isActive,
        ,
        ,
        bool isPaused,
        DataTypes.AssetType reserveAssetType
    ) = reserveCache.reserveConfiguration.getFlags();

    require(
        reserveAssetType == DataTypes.AssetType.ERC20,
        Errors.INVALID_ASSET_TYPE
    );
    require(isActive, Errors.RESERVE_INACTIVE);
    require(!isPaused, Errors.RESERVE_PAUSED);
}

When depositing a BAYC, MAYC and BAKC sAPE is not immediately enabled as collateral. It is only enabled when a user calls PoolApeStaking#borrowApeAndStake. Otherwise if the user tries to enable manually enable it as a collateral if will revert in ValidationLogic#validateSetUseERC20AsCollateral. The result is that if a user deposits an NFT already staked with APE, then it won't be accounted for in their health factor. This is possible because when APE is staked it is locked in the staking contract but the NFT is free to be transferred link. This can lead to users being unfairly liquidated and their collateral being sold for a large loss. They will also be force to pay a liquidation incentive even though their position isn't actually under-collateralized

Tools Used

Manual Review

Recommended Mitigation Steps

When a user deposits a BAYC, MAYC or BAKC the contract should check if the NFT already has sAPE. If it does then the contract should automatically enable the sAPE as collateral.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/libraries/ApeStakingLogic.sol#L156-L185

Vulnerability details

Impact

User is forced to pay too high of an incentive if part of their staked apecoin isn't borrowed and they are forced to unstake

Proof of Concept

    uint256 unstakedAmount = _apeCoin.balanceOf(address(this)) -
        balanceBefore;
    if (unstakedAmount == 0) {
        return;
    }
    //2 send incentive to caller
    if (params.incentiveReceiver != address(0)) {
        uint256 unstakeIncentive = stakingParameter.unstakeIncentive;
        if (unstakeIncentive > 0) {
            uint256 incentiveAmount = unstakedAmount.percentMul(
                unstakeIncentive
            );
            _apeCoin.safeTransfer(
                params.incentiveReceiver,
                incentiveAmount
            );
            unstakedAmount = unstakedAmount - incentiveAmount;
        }
    }

Paraspace allows users to deposit their MAYC, BAYC and BAKC then take out a loan against their NFT to borrow apecoin and stake it. The staked coins don't have to be borrowed. A user can stake ApeCoin before they deposit their NFT to ParaSpace. If their collateralization drops too low they will be forced to unstake and will be force to pay an incentive to the caller. This is where the issues is because when forced to unstake they will pay an incentive on ApeCoin that wasn't borrowed.

Tools Used

Manual Review

Recommended Mitigation Steps

Rewrite the lines so that the user only pays incentive on the amount that was liquidated so that user is not unfairly penalized:

    //2 repay 
    DataTypes.ReserveData memory apeCoinData = params.POOL.getReserveData(
        address(_apeCoin)
    );
    uint256 repayAmount = IERC20(apeCoinData.variableDebtTokenAddress)
        .balanceOf(positionOwner);
    if (repayAmount > 0) {
        repayAmount = Math.min(repayAmount, unstakedAmount);
    }
    uint256 supplyAmount = unstakedAmount - repayAmount;

    //3 send incentive to caller and supply
    if (params.incentiveReceiver != address(0)) {
        uint256 unstakeIncentive = stakingParameter.unstakeIncentive;
        if (unstakeIncentive > 0) {
            uint256 incentiveAmount = repayAmount.percentMul(
                unstakeIncentive
            );
            _apeCoin.safeTransfer(
                params.incentiveReceiver,
                incentiveAmount
            );
            supplyAmount = supplyAmount - incentiveAmount;
        }
    }