The onERC721Received function of V3Vault.sol is invoked whenever a new position is created or an existing one is transformed. In the case of AutoRange.sol transformer, the current user position is replaced with a new one, transferring back the old one to its owner. Since the the old position NFT was transferred in the same transaction, it opened the door for re-entrancy exploits
Mitigation
PR-8 successfully mitigates the original issue by implementing the pull over push pattern when returning position NFTs to their owners:
the _cleanupLoan() function used inside onERC721Received() to send the old position to its owner has been refactored so that it only clears the position from the loan, without sending it directly.
a new remove() function is introduced, so that users can withdraw themselves their position NFT from the protocol
refactoring _cleanupLoan() also made safer the other 2 functions that were using it - repay() & liquidate()
Due to the changes, reentrancy is no longer possible when replacing positions.
Lines of code
Vulnerability details
C4 Issue
H-02: Risk of reentrancy onERC721Received function to manipulate collateral token configs shares
Issue Details
The
onERC721Received
function ofV3Vault.sol
is invoked whenever a new position is created or an existing one is transformed. In the case ofAutoRange.sol
transformer, the current user position is replaced with a new one, transferring back the old one to its owner. Since the the old position NFT was transferred in the same transaction, it opened the door for re-entrancy exploitsMitigation
PR-8 successfully mitigates the original issue by implementing the
pull over push
pattern when returning position NFTs to their owners:_cleanupLoan()
function used insideonERC721Received()
to send the old position to its owner has been refactored so that it only clears the position from the loan, without sending it directly.remove()
function is introduced, so that users can withdraw themselves their position NFT from the protocol_cleanupLoan()
also made safer the other 2 functions that were using it -repay()
&liquidate()
Due to the changes, reentrancy is no longer possible when replacing positions.
Comment
Small QA detail I noticed is that during refactoring a check got duplicated two times in the
audit
branch https://github.com/revert-finance/lend/blob/dcfa79924c0e0ba009b21697e5d42d938ad9e5e3/src/V3Vault.sol#L1052-L1058Consider removing the duplicate
Conclusion
Mitigation Confirmed