Revert Lend - novamanbg's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/V3Utils.sol#L98

Vulnerability details

Impact

Malicious users can change other user's position by calling execute in V3Utils.sol and earn tokens - High severity

Proof of Concept

Consider the following scenario:

1. Alice calls executeWithPermit and makes a change in her position. This function approves the V3Utils contract to her position NFT, however this approval is not reset after execution.
2. Bob is a malicious user and sees Alices transaction. He knows that the V3Utils contract is now approved her NFT.
3. Bob calls execute, passing Alice's tokenId. With instructions to decrease liquidity and withdraw and collect the tokens, from the decrease. As a result Bob earned tokens by changing Alice's position.

The execute function in V3Utils.sol does not have any access control so any approved tokens to the V3Utils contract are put in danger.

Tools Used

Manual Review

Recommended Mitigation Steps

Access control should be added to the execute function in V3Utils.

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Vault.sol#L685

Vulnerability details

Impact

In V3Vault.sol user can create a position that cannot be liquidated, simply by implementing a onERC721Received function of the NFT owner contract, causing safeTransferFrom to revert - High severity.

Proof of Concept

The liquidate function in V3Vault.sol makes an internal call to _cleanUpLoan. https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Vault.sol#L744 _cleanUpLoan uses safeTransferFrom() to transfer the NFT back to the owner. https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Vault.sol#L1083 safeTransferFrom will always revert if the onERC721Received owner function does not return the acceptance magic value to accept the transfer. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/utils/ERC721Utils.sol?source=post_page-----8a1fb636fcd1--------------------------------#L22-L47 As a result a user could create a onERC721Received that does not return the acceptance magic value causing the whole liquidate function to revert.

There are two cases in which the safeTransferFrom will revert:

1. If the user has not implemented the onERC721Received
2. If the user has the function but does not return the IERC721Receiver.onERC721Received.selector in all cases

Tools Used

Manual Review

Recommended Mitigation Steps

The solution would be to use transferFrom() which will never revert.

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/automators/AutoExit.sol#L200 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/automators/Automator.sol#L221

Vulnerability details

Impact

Malicious actor can launch a gas griefing attack on a operator executing execute in AutoExit.sol. Since griefing attacks have no economic incentive for the attacker and it also requires operators it should be Medium severity.

Proof of Concept

The execute function is meant to be called by a "revert controlled bot (operator)". However the execute function in AutoExit.sol calls _transferToken which will perform the following unsafe external call if the token being passed is WETH:

(bool sent,) = to.call{value: amount}("");

Now (bool sent, ) is actually the same as writing (bool sent, bytes memory data) which basically means that even though the data is omitted it doesn’t mean that the contract does not handle it. Actually, the way it works is the bytes data that was returned from the "to" address will be copied to memory. Memory allocation becomes very costly if the payload is big, so this means that if a "to address" implements a fallback function that returns a huge payload, then the msg.sender of the transaction, in our case the "revert controlled bot (operator)", will have to pay a huge amount of gas for copying this payload to memory.

Tools Used

Manual Review

Recommended Mitigation Steps

Use a low-level assembly call since it does not automatically copy return data to memory

bool success;
assembly {
    success := call(gasLimit, to, amount, 0, 0, 0, 0)
}

Assessed type

ETH-Transfer