AbraNFT contest - antonttc's results

Lines of code

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

Vulnerability details

Impact

Loan can be destroyed because of no price reported by oracle. (or any other reason that makes oracle "work as expected" in bad situations when the get function return success=false)

Proof of Concept

According to the interface of INFTOracle, the first parameter returned is boolean success indicating if the request is successful. However, this boolean is not checked before rate is used to determine if the loan is underCollateralized in NFTPairWithOracle (there are 2 places where use oracle.get without checking success or not). This means, the oracle can be "operating normally", and already indicated they can't give a price at the moment by returning success=false, rate=0. But in this scenario, rate = 0 will be used and all loans will be determined as under collateralized.

Recommended Mitigation Steps

(bool sucess, uint256 rate) = loanParams.oracle.get(address(this), tokenId);
require(success, "Oracle not up to date");

Gas optimizations:

1. Unnecessary loan parameter comparison in _lend when used in through requestAndBorrow or takeCollateralAndLend

• in internal function _lend, it takes an parameter called accepted and compare it with the state variable tokenLoanParams to make sure the borrower didn't front-run the tx. This is a good check, but in requestAndBorrow and takeCollateralAndLend, same parameters are passed in for _requestLoan and _lend, considering these are probably the most used function, it would be better to use a boolean call checkLoan to decide if we need this check or not. It will only be needed in lend function. (or just move the check into a different internal function and call it before lend)

QA Report: Recommandation (No bug found)

I highly recommend the team to remove the ability to do arbitrary function calls on behalf of the NFTPair contract. Use of arbitrary contract call is a very dangerous pattern, it should be avoided unless there's a very strong motive to support it.

Things that can go wrong with it:

1. user's wallet can be drained if they accidentally approve the NFTPair contract to use their asset. (it should have been approving BentoBox) This can be achieved by calling asset.transferFrom(user, attacker).
2. potential airdrops or any token will become public for anyone to withdraw.
3. it's possible that the collateral suffer the same vulnerability as the tUSD bug and could have lead to huge hack in Compound. The exact bug caused by having 2 entry points to a single contract is unlikely to happen with well-known ERC20s again, but there might be more potential issue similar to this one, and the process of adding collateral will need to be better reviewed, potentially the team should not accept any upgradable contracts.
4. Same thing happen to BentoBoxV1 contract, if the team decide to launch this code base on BentoBoxV1 it should be safe, but if the newer version is upgrdable then this mechanism cannot be trusted, because it's possible that user use different entry points to mess up BentoBox's accounting.

Solution:

If the team added this feature to support token farming or trapped tokens, it would make sense to add onlyOwner sweep function to do that; If it was mainly for interacting with other protocols, I suggest add a custom interface as a middle layer: So only allow the NFTPair contract to call ICallee(callee).ourSpecialCall(data), and for each cool feature you want to support you add a new callee contract which works as a security proxy.

IMO arbitrary calls lead to a huge space of potential vulnerabilities (some of them might still be unknown), so i would suggest to completely remove the possibility of volnerability by replacing it with one of the solution above.

Gas optimizations:

1. Unnecessary loan parameter comparison in _lend when used in through requestAndBorrow or takeCollateralAndLend

• in internal function _lend, it takes an parameter called accepted and compare it with the state variable tokenLoanParams to make sure the borrower didn't front-run the tx. This is a good check, but in requestAndBorrow and takeCollateralAndLend, same parameters are passed in for _requestLoan and _lend, considering these are probably the most used function, it would be better to use a boolean call checkLoan to decide if we need this check or not. It will only be needed in lend function. (or just move the check into a different internal function and call it before lend)