AbraNFT contest - GimelSec's results

Lines of code

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

Vulnerability details

Impact

A lender (attacker) can modify the oracle address of tokenLoanParams to get collateral directly before expiration.

Proof of Concept

1. First a lender (attacker) lends for a loan, the attacker can use updateLoanParams function and update to a malicious oracle address (because the updateLoanParams function doesn’t check the validation of oracle address).
2. Before expiration of the loan, the attacker can call the removeCollateral function to bypass the NFT value check (require of L288) by using the malicious oracle to get a very low rate (e.g. 0) and get the collateral directly.

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

Tools Used

vim

Recommended Mitigation Steps

Check the original oracle address and params.oracle address should be the same. It should not update the oracle address in the updateLoanParams function.

Lines of code

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

Vulnerability details

Impact

A borrower (attacker) can use reentrancy attack to request a loan successfully and the collateral is still owned by the attacker. If a lender (victim) tries to call lend on the malicious loan which seems normal, the lender will lose money and never get the collateral.

Proof of Concept

Some ERC721 tokens implement the _beforeTokenTransfer function which may call another address that is controllable by msg.sender. A borrower (attacker) can use the feature to trigger reentrancy attack:

1. First, the borrower (attacker) calls requestLoan normally with tokenId 1. Now tokenId 1 is owned by NFTPair contract.
2. Then the attacker calls removeCollateral to trigger reentrancy attack on L266.
3. L266 is collateral.transferFrom(address(this), to, tokenId);, the attacker uses _beforeTokenTransfer of the collateral to re-entry requestLoan with skim = true.
4. Because tokenId 1 is owned by NFTPair contract and L265 delete tokenLoan[tokenId]; initialize tokenLoan[tokenId] (including loan status), the attacker will re-entry requestLoan successfully with skim = true and create a new loan of tokenLoan[tokenId].
5. After the reentrancy, L266 will transfer tokenId 1 back to the attacker. Now the attacker creates a loan of tokenId 1, but the token is still owned by the attacker.

There are some attack scenarios:

• If a lender (victim) tries to call lend on the malicious loan which seems normal, the lender will lose money and never get the collateral.
• In another scenario, the borrower (attacker) can first create a normal loan. When a lender (victim) calls lend, the attacker can frontrun the reentrancy attack before the lend called by the victim.
• It also works if a lender is malicious, the lender can use the reentrancy attack on removeCollateral to create a malicious loan.
• Also the repay function suffers from this reentrancy attack.

Tools Used

vim, ethers.js

Recommended Mitigation Steps

Delete tokenLoan after collateral.transferFrom:

        …
        collateral.transferFrom(address(this), to, tokenId);
        delete tokenLoan[tokenId];
}

or use ReentrancyGuard.

Summary

We list 1 non-critical finding:

• (Non) Using ecrecover is against best practice

(Non) Using ecrecover is against best practice

Impact

Using ecrecover is against best practice. Preferably use ECDSA.recover instead. EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature unique. However it should be impossible to be a threat by now.

Proof of Concept

NFTPair.sol
383:            require(ecrecover(_getDigest(dataHash), v, r, s) == lender, "NFTPair: signature invalid");
419:        require(ecrecover(_getDigest(dataHash), v, r, s) == borrower, "NFTPair: signature invalid");

NFTPairWithOracle.sol
417:            require(ecrecover(_getDigest(dataHash), signature.v, signature.r, signature.s) == lender, "NFTPair: signature invalid");
452:        require(ecrecover(_getDigest(dataHash), signature.v, signature.r, signature.s) == borrower, "NFTPair: signature invalid");

Recommended Mitigation Steps

Take these implementation into consideration

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4a9cc8b4918ef3736229a5cc5a310bdc17bf759f/contracts/utils/cryptography/draft-EIP712.sol https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/draft-ERC20Permit.sol

Save gas in for loops by ++i rather than i++

In for loops, using ++i rather than i++ to save gas.

Proof of Concept

NFTPair.sol
494:        for (uint256 k = 2; k <= COMPOUND_INTEREST_TERMS; k++) {
641:        for (uint256 i = 0; i < actions.length; i++) {

NFTPairWithOracle.sol
527:        for (uint256 k = 2; k <= COMPOUND_INTEREST_TERMS; k++) {
674:        for (uint256 i = 0; i < actions.length; i++) {

Recommendation

Use ++i rather than i++ to save gas.