AbraNFT contest - catchup's results

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L287 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L321

Vulnerability details

Impact

_lend() and removeCollateral() functions use oracle.get() to get the value from the oracle. get() function returns 2 values; success and rate. Success indicates if there is a valid rate information for the pair. However, success value is not taken into consideration. Therefore, calculations may be done using a faulty rate data.

Proof of Concept

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L287 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L321

Tools Used

Manual analysis

Recommended Mitigation Steps

I'd suggest to include a check for the success and discard the rate information if success fails.

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L209 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L286-L288

Vulnerability details

Impact

Lender can update the loan terms using updateLoanParams() function. As it commented in the function: "The lender can change terms so long as the changes are strictly the same or better for the borrower" Regarding the LTV, condition is that the new LTV must be less than or equal to the current LTV: params.ltvBPS <= cur.ltvBPS, Lender has the right to seize the collateral if the loan is underwater. This check is done by the code below: uint256 amount = loanParams.valuation + interest; (, uint256 rate) = loanParams.oracle.get(address(this), tokenId); require(rate.mul(loanParams.ltvBPS) / BPS < amount, "NFT is still valued"); Hence, the lender can effectively downgrade the LTV, turn a valid loan to underwater and seize the collateral.

Proof of Concept

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L209 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L286-L288

Tools Used

Manual analysis

Recommended Mitigation Steps

The condition should be changed as: params.ltvBPS >= cur.ltvBPS,

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L316

Vulnerability details

Impact

It comments in the _lend() function that lender accepted conditions must be at least as good as the borrower is asking for. The line which checks the accepted LTV (lender's LTV) against borrower asking LTV is: params.ltvBPS >= accepted.ltvBPS, This means lender should be offering a lower LTV, which must be the opposite way around. I think this may have the potential to strand the lender, if he enters a lower LTV. For example borrower asking LTV is 86%. However, lender enters his accepted LTV as 80%. lend() will execute with 86% LTV and punish the lender, whereas it should revert and acknowledge the lender that his bid is not good enough.

Proof of Concept

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L316

Tools Used

Manual analysis

Recommended Mitigation Steps

The condition should be changed as: params.ltvBPS <= accepted.ltvBPS,

Old solidity version

Contracts are using 0.6.12 compiler which is a bit outdated. It would be better to use a version >= 0.8.4 to benefit from new features such as builtin safemath operations and bugfixes.

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L20 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L20

Recommended Mitigation Steps

I suggest to use at least compiler 0.8.4

Missing 0 address check

It is a good practice to include 0 address check while updating an important address. I suggest to include a zero address check for setFeeTo() function.

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L728 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L750

Some of the public state variables can be private

Private state variables are cheaper than public state variables. There are some instances where public variables can be private.

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L76 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L77 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L84 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L85 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L91 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L94 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L165 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L93 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L94 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L101 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L102 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L108 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L111 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L182

Some internal functions can be made private

Calling private functions are cheaper than calling internal functions. Therefore, it is better to declare functions private if they are not called from inherited contracts.

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L276 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L573 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L583 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L595 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L608 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L276 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L305 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L606 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L616 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L628 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L641

Recommended Mitigation Steps

Change the visibility to private where possible.

Redundant initialisation with default value

Some variables are initialised with their default values which cause unnecessary gas consumption

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L96 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L641 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L113 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L674

Error strings longer than 32 characters

Error reason strings take space in the deployed bytecode. Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L366 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L398

Prefix increments are cheaper than postfix

Prefix increments are cheaper than postfix increments. So i++ can be replaced by ++i at the lines below.

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L494 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L641 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L527 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L674

Cached variable can be eliminated

amount variable is used only once. It can be deleted and replaced by "loanParams.valuation + interest" where amount is used.

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L286-L288

Recommended Mitigation Steps

Code can be changed as: // uint256 amount = loanParams.valuation + interest; (, uint256 rate) = loanParams.oracle.get(address(this), tokenId); require(rate.mul(loanParams.ltvBPS) / BPS < (loanParams.valuation + interest), "NFT is still valued");