H-02 MitigationConfirmed

[c4-bot-8]

Lines of code

Vulnerability details

C4 issue

H-02: Risk of re-entrancy onERC721Received function to manipulate collateral token configs shares

Comments

V3Vault.transform() will transfer a NFT position to the original NFT owner when it's cleaning up an old loan that is being transferred to a new loan. Because of this transfer, the NFT owner will receive the NFT and will be able to execute arbitrary code that can lead to a re-entrancy. This re-entrancy grants the NFT owner the ability to manipulate the state of the V3Vault contract. Once the re-entrancy transaction is complete, the parent transaction will continue to execute operations (i.e. V3Vault._updateAndCheckCollateral()). With a manipulated state through re-entrancy and V3Vault._updateAndCheckCollateral() being called, the state of the contract will become corrupted.

As specified in the primary issue, the problem arises because Revert does not apply a CEI pattern to V3Vault.transform(). Because there is no re-entrancy protection in the contract, the malicious user can call other functions in V3Vault, such as borrow(), and manipulate the token config total debt shares. This manipulation will lead to permanent corruption of the token's total debt shares, breaking the contract functionality.

Mitigation

PR #8, #32

The mitigation involves several fixes:

• V3Vault.remove() function is added. This function will send a NFT position for a closed loan back to the owner. It does so by doing the following:
  • Check to see if msg.sender is the tokenOwner
  • Check to see if the debtShares for the loan is zero. If it's not zero, this indicates that the loan is open and can not be removed. The function will then revert. If it's zero, the operation will continue.
  • The token is removed from the owner via calling _removeTokenFromOwner().
  • The token is safe transferred to the recipient.
• V3Vault._repay() has a guard check to make sure that more than 0 shares are being repaid. If the shares equal 0, the function will revert. This is expected behavior as a repayment of 0 does not make sense and should not be allowed.
• V3Vault._repay() removes a call to _cleanupLoan() when the debt is fully repaid. This is expected behavior because now that V3Vault.remove() exists, the protocol no longer needs to call _cleanupLoan(). The _updateAndCheckCollateral() function is still called and once V3Vault._repay() is complete, users can call V3Vault.remove() to retrieve their token.
• V3Vault._cleanupLoan() no longer removes the token from the owner nor transfers the token back to the owner. Removing the NFT transfer prevents the NFT re-entrancy. Users are still able to retrieve their token in a separate call to V3Vault.remove() which is not vulnerable to re-entrancy due to it following the CEI pattern.

In summary, when a loan is cleaned up, the token is no longer transferred to the user. This core fix prevents a re-entrancy from occuring.

Anything Else We Should Know

Although this is not strictly an issue, Revert has duplicated the guard check in _repay(). This can be seen below:

https://github.com/revert-finance/lend/blob/audit/src/V3Vault.sol?plain=1#L1052-L1058

Finally, V3Vault.remove() does not delete the loan from the loans mapping. This can lead to orphaned loans existing in the state when a user fully repays a loan and then calls remove(). There is no known bug due to this behavior but can result in more expensive gas down the road as more orphaned loans persist.

Conclusion

LGTM

[c4-judge]

jhsagd76 marked the issue as satisfactory

[c4-judge]

jhsagd76 marked the issue as confirmed for report