search

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

2 stars 0 forks source link

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

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_);

2023-05-ajna/ajna-core/src/RewardsManager.sol

Lines 207 to 260 in 276942b function stake( uint256 tokenId ) external override { address ajnaPool = PositionManager(address(positionManager)).poolKey(tokenId); 

 // check that msg.sender is owner of tokenId 
 if (IERC721(address(positionManager)).ownerOf(tokenId_) != msg.sender) revert NotOwnerOfDeposit(); 

 StakeInfo storage stakeInfo = stakes[tokenId_]; 
 stakeInfo.owner    = msg.sender; 
 stakeInfo.ajnaPool = ajnaPool; 

 uint256 curBurnEpoch = IPool(ajnaPool).currentBurnEpoch(); 

 // record the staking epoch 
 stakeInfo.stakingEpoch = uint96(curBurnEpoch); 

 // initialize last time interaction at staking epoch 
 stakeInfo.lastClaimedEpoch = uint96(curBurnEpoch); 

 uint256[] memory positionIndexes = positionManager.getPositionIndexes(tokenId_); 

 for (uint256 i = 0; i < positionIndexes.length; ) { 

     uint256 bucketId = positionIndexes[i]; 

     BucketState storage bucketState = stakeInfo.snapshot[bucketId]; 

     // record the number of lps in bucket at the time of staking 
     bucketState.lpsAtStakeTime = uint128(positionManager.getLP( 
         tokenId_, 
         bucketId 
     )); 
     // record the bucket exchange rate at the time of staking 
     bucketState.rateAtStakeTime = uint128(IPool(ajnaPool).bucketExchangeRate(bucketId)); 

     // iterations are bounded by array length (which is itself bounded), preventing overflow / underflow 
     unchecked { ++i; } 
 } 

 emit Stake(msg.sender, ajnaPool, tokenId_); 

 // transfer LP NFT to this contract 
 IERC721(address(positionManager)).transferFrom(msg.sender, address(this), tokenId_); 

 // calculate rewards for updating exchange rates, if any 
 uint256 updateReward = _updateBucketExchangeRates( 
     ajnaPool, 
     positionIndexes 
 ); 

 // transfer rewards to sender 
 _transferAjnaRewards(updateReward);

}

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.

2023-05-ajna/ajna-core/src/RewardsManager.sol

Lines 270 to 303 in 276942b function unstake( uint256 tokenId ) external override { StakeInfo storage stakeInfo = stakes[tokenId]; 

 if (msg.sender != stakeInfo.owner) revert NotOwnerOfDeposit(); 

 address ajnaPool = stakeInfo.ajnaPool; 

 // claim rewards, if any 
 _claimRewards( 
     stakeInfo, 
     tokenId_, 
     IPool(ajnaPool).currentBurnEpoch(), 
     false, 
     ajnaPool 
 ); 

 // remove bucket snapshots recorded at the time of staking 
 uint256[] memory positionIndexes = positionManager.getPositionIndexes(tokenId_); 
 for (uint256 i = 0; i < positionIndexes.length; ) { 
     delete stakeInfo.snapshot[positionIndexes[i]]; // reset BucketState struct for current position 

     unchecked { ++i; } 
 } 

 // remove recorded stake info 
 delete stakes[tokenId_]; 

 emit Unstake(msg.sender, ajnaPool, tokenId_); 

 // transfer LP NFT from contract to sender 
 IERC721(address(positionManager)).transferFrom(address(this), msg.sender, tokenId_);

}

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