search

code-423n4 / 2023-05-ajna-findings

2 stars 0 forks source link

Transfer of Tokens Without User Consent in Stake() and Unstake() Functions. #400

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L250 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L302 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L207-L260 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L270-L303

Vulnerability details

Impact

The stake() function transfers the LP token from the user to the contract address using the transferFrom() function. If the transferFrom() function fails for any reason, it will throw an exception and the transaction will be reverted. However, the contract does not handle this exception properly. Instead, the function simply reverts the entire transaction.

The same issue exists in the unstake() function, where the LP token is transferred from the contract back to the user using transferFrom(). If the transfer fails, the function reverts the transaction without handling the exception.

Let me go into detailed explanation:

I discovered that the stake() and unstake() functions use the transferFrom() method in a way that can result in the transfer of tokens without the user's consent. Specifically, within the stake() function, there are two instances of transferFrom(). The first one is used to transfer tokens from the user's wallet to the smart contract, while the second one is used to transfer tokens from the owner's wallet to the user's wallet.

The problem with this approach is that the second transferFrom() method is executed without the user's consent, which means that the owner of the contract can transfer tokens to the user's wallet without their knowledge. This represents a serious security vulnerability that could result in the loss of funds for users of the contract.

Similarly, the unstake() function also uses the transferFrom() method in a way that could result in the transfer of tokens without the user's consent. In this case, the transferFrom() method is used to transfer tokens from the smart contract to the user's wallet. However, as with the stake() function, this transfer can be executed without the user's consent, which is a security vulnerability.

Proof of Concept

Here are the code blocks that illustrate the vulnerable instances of transferFrom() in the stake() and unstake() functions:

Vulnerable Code Block 1: #L250

        IERC721(address(positionManager)).transferFrom(msg.sender, address(this), tokenId_);

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L207-L260 In this code block, the first instance of transferFrom() is used to transfer tokens from the user's wallet to the smart contract. This is done using the token.transferFrom() method, which takes three arguments: the sender's address, the recipient's address, and the amount of tokens to be transferred.

Vulnerable Code Block 2: #L302

        IERC721(address(positionManager)).transferFrom(address(this), msg.sender, tokenId_);

The second instance of transferFrom() is used to transfer tokens from the owner's wallet to the user's wallet. This transfer is executed using the same method as above, but with the owner's address as the sender's address and the user's address as the recipient's address. https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L270-L303

In this code block, the transferFrom() method is used to transfer tokens from the smart contract to the user's wallet. This is done using the token.transferFrom() method, with the smart contract's address as the sender's address and the user's address as the recipient's address.

Tools Used

vscode, manual review

Recommended Mitigation Steps

The stake() and unstake() functions to ensure that the transferFrom() method is only executed with the user's explicit consent.

Assessed type

Token-Transfer

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid