Taiko - pa6kuda's results

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/team/airdrop/ERC20Airdrop2.sol#L104-L122 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/team/airdrop/ERC20Airdrop2.sol#L88-L94 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/team/airdrop/ERC20Airdrop2.sol#L39-L44

Vulnerability details

Summary

ERC20Airdrop2.getBalance() function calculates the reward based on the time that has passed after claimEnd. To get full reward time should be more or equal to claimEnd + withdrawalWindow. It is obvious that the user will try to make one transaction at the end of this period to get a full reward, but it's almost impossible. It's because if user spends an additional 1 second, executing ERC20Airdrop2.withdraw() would revert because it has ERC20Airdrop2.ongoingWithdrawals() modifier. The condition of this modifier says that user can not call function if period in which reward accumulates (claimEnd + withdrawalWindow) has passed. That means user has only 1 second to withdraw full reward, so when user executes ERC20Airdrop2.withdraw() block.timestamp must be equal to claimEnd + withdrawalWindow. User can not predict at what timestamp transaction will be executed. Also, we need to remember that block.timestamp can be manipulated by miners in their favour (for example, your transaction can be specially delayed (not for a long time) to make block.timestamp more then claimEnd + withdrawalWindow).

Impact

User can not withdraw full reward that was given to him in ERC20Airdrop2.

Proof of Concept

This test shows that user can not withdraw full reward when he is late even by one second.

Add this test to ERC20Airdrop2.t.sol and run with

    forge test --match-contract TestERC20Airdrop2 --match-test test_withdraw_user_can_withdraw_full_amount_only_if_withdraw_is_executed_in_last_second

    function test_withdraw_user_can_withdraw_full_amount_only_if_withdraw_is_executed_in_last_second()
        public
    {
        vm.warp(claimStart + 1);

        // variable  get from test setUp when deploying airdrop2
        uint256 withdrawalWindow = 10 days;
        uint256 fullRewardTime = claimEnd + withdrawalWindow;

        // Alice claimed 100e18 tokens
        vm.prank(Alice, Alice);
        airdrop2.claim(Alice, 100e18, merkleProof);

        // average user needs at least 5 minutes to make transaction (time to open app, time to login into wallet, time to approve transaction and  execution time of transaction)
        vm.roll(block.number + 200);
        vm.warp(fullRewardTime - 5 minutes); // 5 minutes before full reward will be accumulated

        (, uint256 withdrawable) = airdrop2.getBalance(Alice);

        // shows that withdrawable is less than balance
        console.log(withdrawable); // 99965277777777777777

        // the only time (block.timestamp) when user can withdraw full reward
        vm.roll(block.number + 200);
        vm.warp(fullRewardTime);

        (, withdrawable) = airdrop2.getBalance(Alice);

        console.log(withdrawable); // 100000000000000000000

        // when 1 or more seconds pass user can no longer withdraw full reward
        vm.roll(block.number + 200);
        vm.warp(fullRewardTime + 1);

        // so withdraw will revert
        vm.expectRevert(ERC20Airdrop2.WITHDRAWALS_NOT_ONGOING.selector);
        airdrop2.withdraw(Alice);
    }

Recommended Mitigation Steps

Remove second condition of ERC20Airdrop2.ongoingWithdrawals(), so user can call ERC20Airdrop2.withdraw() after reward are fully accumulated, or add additional time so user can withdraw reward without problem.

@@ -37,7 +37,7 @@ contract ERC20Airdrop2 is MerkleClaimable {
     error WITHDRAWALS_NOT_ONGOING();
 
     modifier ongoingWithdrawals() {
-        if (claimEnd > block.timestamp || claimEnd + withdrawalWindow < block.timestamp) {
+        if (claimEnd > block.timestamp || claimEnd + withdrawalWindow + 1 days < block.timestamp) {
             revert WITHDRAWALS_NOT_ONGOING();
         }
         _;

Assessed type

Invalid Validation

[QA-01] BridgedERC1155 tokens without name/symbol can confuse users as they will have same name/symbol.

Relevant GitHub Links

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/BridgedERC1155.sol#L50-L51

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/BridgedERC1155.sol#L115-L117

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/LibBridgedToken.sol#L28-L37

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/BridgedERC1155.sol#L121-L123

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/LibBridgedToken.sol#L39-L41

Summary

As described in BridgedERC1155.init() function

The symbol and the name can be empty for ERC1155 tokens so we use some placeholder data for them instead.

But these placeholders are the same and they aren't used to override name/sybmol if either of them is not provided. That means functions BridgedERC1155.name() and BridgedERC1155.symbol() will return the same data for other BridgedERC1155 that don't have names and/or symbols which can confuse users, especially on some NFT marketplace.

Recommended Mitigation Steps

Add some default name that will not confuse user, or add and display a unique id for every deployed ERC1155 when calling ERC1155Vault._handleMessage().

[QA-02] Use existing _inNonReentrant() function in EssentialContract.nonReentrant() modifier.

Relevant GitHub Links

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/common/EssentialContract.sol#L46-L51

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/common/EssentialContract.sol#L140-L142

Summary

EssentialContract already have function that checks it reentry slot has value of _TRUE or _FALSE, so there is no need to recreate the same logic in EssentialContract.nonReentrant() modifier.

Recommended Mitigation Steps

Use EssentialContract._inNonReentrant() instead of _loadReentryLock() == _TRUE

     modifier nonReentrant() {
-        if (_loadReentryLock() == _TRUE) revert REENTRANT_CALL();
+        if (_inNonReentrant()) revert REENTRANT_CALL();
         _storeReentryLock(_TRUE);
         _;
         _storeReentryLock(_FALSE);
     }

[QA-03] Rename TimelockTokenPool.getMyGrant() and TimelockTokenPool.getMyGrantSummary() functions as they don't correspond to their's actions.

Relevant GitHub Links

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/team/TimelockTokenPool.sol#L204-L206

Summary

Function TimelockTokenPool.getMyGrant() have name that doesn't correspond to its actions, because it doesn't get message sender's grant, instead, it gets grant of the passed address. Same with TimelockTokenPool.getMyGrantSummary().

Recommended Mitigation Steps

Rename TimelockTokenPool.getMyGrant() on, for example, getGrant and TimelockTokenPool.getMyGrantSummary() on, for example, getGrantSummary:

-    function getMyGrant(address _recipient) public view returns (Grant memory) {
+    function getGrant(address _recipient) public view returns (Grant memory) {
         return recipients[_recipient].grant;
     }

[QA-04] Use existing whenNotPaused modifier in BridgedERC721._beforeTokenTransfer()

Relevant GitHub Links

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/BridgedERC721.sol#L115-L128

Summary

EssentialContract that is the parent of BridgedERC721 and BridgedERC1155 contracts already have whenNotPaused modifier that should be used instead of if (paused()) revert INVALID_PAUSE_STATUS(); as it's just a redundant code.

Recommended Mitigation Steps

    function _beforeTokenTransfer(
        address, /*_from*/
        address _to,
        uint256, /*_firstTokenId*/
        uint256 /*_batchSize*/
    )
         internal
         virtual
         override
+        whenNotPaused
     {
         if (_to == address(this)) revert BTOKEN_CANNOT_RECEIVE();
-        if (paused()) revert INVALID_PAUSE_STATUS();
     }
    }

[QA-05] Create function in BaseVault contract that returns destination owner.

Relevant GitHub Links

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/ERC20Vault.sol#L227

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/ERC721Vault.sol#L50

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/ERC1155Vault.sol#L64

Summary

Code part _op.destOwner != address(0) ? _op.destOwner : msg.sender is used in multiple places, in that case, it should be separated into function.

Recommended Mitigation Steps

Create a separate function in BaseVault contract

+    /// @notice Get's address of destination owner
+    /// @param _destOwner The address that can be address(0)
+    /// @return _destOwner if destOwner is not address(0), in other case msg.sender
+    function getDestinationOwner(address _destOwner) internal view returns (address) {
+        return _destOwner != address(0) ? _destOwner : msg.sender;
+    }