bin2chen - resolveQueuedTrades() ERC777 re-enter to steal funds

[sherlock-admin]

bin2chen

medium

resolveQueuedTrades() ERC777 re-enter to steal funds

Summary

_openQueuedTrade() does not follow the “Checks Effects Interactions” principle and may lead to re-entry to steal the funds

https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html

Vulnerability Detail

The prerequisite is that tokenX is ERC777 e.g. “sushi”

1. resolveQueuedTrades() call _openQueuedTrade()
2. in _openQueuedTrade() call "tokenX.transfer(queuedTrade.user)" if (revisedFee < queuedTrade.totalFee) before set queuedTrade.isQueued = false;

    function _openQueuedTrade(uint256 queueId, uint256 price) internal {
...
        if (revisedFee < queuedTrade.totalFee) {
            tokenX.transfer( //***@audit call transfer , if ERC777 , can re-enter ***/
                queuedTrade.user,
                queuedTrade.totalFee - revisedFee
            );
        }

        queuedTrade.isQueued = false;  //****@audit  change state****/
    }

3.if ERC777 re-enter to #cancelQueuedTrade() to get tokenX back,it can close, because queuedTrade.isQueued still equal true
4. back to _openQueuedTrade() set queuedTrade.isQueued = false
5.so steal tokenX

Impact

if tokenX equal ERC777 can steal token

Code Snippet

https://github.com/sherlock-audit/2022-11-buffer/blob/main/contracts/contracts/core/BufferRouter.sol#L350

Tool used

Manual Review

Recommendation

follow “Checks Effects Interactions”

    function _openQueuedTrade(uint256 queueId, uint256 price) internal {
...
+      queuedTrade.isQueued = false; 
        // Transfer the fee to the target options contract
        IERC20 tokenX = IERC20(optionsContract.tokenX());
        tokenX.transfer(queuedTrade.targetContract, revisedFee);

-       queuedTrade.isQueued = false; 
        emit OpenTrade(queuedTrade.user, queueId, optionId);
    }

[IAm0x52]

Fixed in PR#8

Changes look good. Trade is now removed from queue before sending user refund during option opening to avoid potential reetrancy. Canceling already removed trade before sending refund so no change needed there.