search

code-423n4 / 2022-04-jpegd-findings

1 stars 1 forks source link

QA Report #179

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Report

Borrower can potentially close liquidated position

This is not an actual issue right now. But, I'm not sure whether you guys are aware of this or not so I decided to include it in the report.

After a liquidator liquidated a position that was insured, the debtPrincipal value is not modified. If it would be, the user would be able to simply close the position to get their collateral back instead of repurchasing it.

Inside closePosition() the current checks whether the debt was repaid using require(_getDebtAmount(_nftIndex) == 0, "position_not_repaid");. Since the debt was paid off and the debtPrincipal value was unmodified, the _getDebtAmount() function returns the principal. A value that is != 0. Thus, the require statement fails. If you had set that value to zero, the user could simply call closePosition() and get their collateral back.

As I said this seems to be an unintended protection. At least, there are no comments suggesting this to be the reason for not setting debtPrincipal to zero.

An easy solution for this to be an explicit block would be to add the following line to the closePosition() function:

require(position.liquidatedAt == 0, "liquidated");

The same thing is done in the repay() function.

Relevant code:

Supposed scenario where vault doesn't hold collateral

The closePosition() function has a comment and logic that suggests that someone could open a position without depositing the ERC721 token to the vault. When reading through the code I couldn't find a scenario where this was expected behaviour.

Other parts of the code where the collateral is transferred this scenario is also ignored. I expect this to be an outdated comment.

Relevant code:

Use call() instead of transfer() to send ETH

transfer() only provides the callee with 2300 gas. For EOA that's not an issue. But, if the receiver is a contract the transfer might fail. Depending on the contract's implementation of the receive() function.

Instead, it's recommended to use call() to transfer Ether.

See https://swcregistry.io/docs/SWC-134

Relevant code:

setNFTTypeValueETH allows setting the floor price

The DAO is able to set the floor price through the overrideFloor() function. It's the designated function for that.

But, the setNFTTypeValueETH() function can also be used to modify the price. It only has an effect, if the floor oracle is already overwritten by the DAO since the function doesn't set the daoFloorOverride value.

Going by the functions and their comments, the setNFTTypeValueETH() function isn't supposed to be able to modify the floor price at all.

Relevant code:

To fix it, add the following require statement: require(_type != bytes32(0));

StrategyPUSDConvex should claim rewards when withdrawing all the tokens

Currently, the withdrawAll() function doesn't claim the rewards. According to the function's comment, it is used primarily to switch strategies. In that case, there is no way for the controller to claim the rewards after the migration besides resetting the strategy to the StrategyPUSDConvex contract.

Since that seems a little tedious and not really wanted, I propose you claim your rewards when withdrawAll() is called. If, there is a scenario where you don't want to claim them you can also introduce a parameter with which you can specify whether to claim or not.

Relevant code:

Proposed Changes

Change withdrawAll() to:

function withdrawAll() external onlyController returns (uint256 balance) {
    address vault = strategyConfig.controller.vaults(address(want));
    require(vault != address(0), "ZERO_VAULT"); // additional protection so we don't burn the funds

    convexConfig.baseRewardPool.withdrawAllAndUnwrap(true);

    balance = want.balanceOf(address(this));
    want.safeTransfer(vault, balance);
}