Check-Effects-Interaction Pattern not followed - Potential Reentrancy Risk

[ShaheenRehman]

Hey, so yesterday I casually tweeted the issue #49 I found a few days ago on twitter: https://twitter.com/0x_Shaheen/status/1743257299530203314

And a Great SR, pointed out that the code is also not following Check-Effects-Interaction (CEI), which poses a re-entrancy risk: https://twitter.com/milotruck/status/1743479319060742566

He was concerned about the Native Tokens reentrancy, which is not possible as .transfer is used to transfer native tokens and due to .transfer's 2300 gas limit, reentrancy is not possible. But there is still a risk for reentrancy in case the non-native token is a ERC777 Token.

An attacker can use claimByTimeout & claimById, to drain the whole vault if the underlying token is an ERC777 Token.

You can read more about the ERC777-Reentracy Issue here: https://blog.openzeppelin.com/exploiting-uniswap-from-reentrancy-to-actual-profit

Mitigation

Code must follow CEI:

    function claimById(
        address from,
        uint256 id,
        uint256 timestamp,
        address token,
        address recipient,
        uint256 amount,
        bool isNative
    ) internal {
        require(hash(abi.encodePacked(from, timestamp, token, recipient, amount)) == deposits[id], "Guard: Invalid id to claim");
        require(amount > 0, "Guard: Invalid amount to claim");
+     delete deposits[id];
        if (isNative) {
            require(IERC20(token).transferFrom(from, address(this), amount), "Guard: claim native token failed");
            uint256 balanceBefore = address(this).balance;
            IWToken(token).withdraw(amount);
            require(address(this).balance == balanceBefore.add(amount), "Guard: token is not wrapped by native token");
            payable(recipient).transfer(amount);
        } else {
            require(IERC20(token).transferFrom(from, recipient, amount), "Guard: claim token failed");
        }
-       delete deposits[id];
        emit TokenClaimed(id);
    }

We would highly reccomend to fix this issue, even if you think that Guard will never interact with ERC777 Tokens. Thanks! Shaheen, Smart Contracts Auditor & Security Researcher.

[hujw77]

Another suggestion is to change transfer to call for the compatible other chains like arbitrum. @xiaoch05

[ShaheenRehman]

Yea, I guess .transfer is compatible with Arbitrum, only ZkSync ERA got some issues with .transfer, afaik. https://medium.com/coinmonks/gemstoneido-contract-stuck-with-921-eth-an-analysis-of-why-transfer-does-not-work-on-zksync-era-d5a01807227d

But yeah, .call is always considered better than .transfer even on mainnet

[xiaoch05]

Yea, I guess .transfer is compatible with Arbitrum, only ZkSync ERA got some issues with .transfer, afaik. https://medium.com/coinmonks/gemstoneido-contract-stuck-with-921-eth-an-analysis-of-why-transfer-does-not-work-on-zksync-era-d5a01807227d

But yeah, .call is always considered better than .transfer even on mainnet

Yeah, it should be better to use the functions defined in TokenTransferHelper instead of token transfer here.