Description

The function withdraw is responsible for the withdrawals of tokens from the escrow contract. The function first checks if there is enough balance in the usersBalance for the caller, then unwraps the token if unwrap is true, and finally decreases the amount from the usersBalance mapping and totalStaked.

If the rebase token is ETH_ADDRESS, the function will unwrap WETH to ETH and send the tokens to the user via the msg.sender.call{value: amount}(""); call. In this function, the checks-effects-interactions pattern is broken, as the interaction (i.e., the transfer of the amount to the caller) is happening before the effects (i.e., the deduction of the amount). Furthermore, the amount is deducted in an unchecked block, which leads to underflow in the balance, and thus the function does not revert when the amount subtracted is more than the balance.

function withdraw(address token, uint256 amount, bool unwrap) external onlyNotBroke returns (uint256) {
    if (amount == 0) revert ZeroAmount();

    EscrowBaseStorage storage s = _getStorage();

    uint256 balance = s.usersBalance[msg.sender][token];
    if (amount > balance) revert NotEnoughAmountStaked(balance); //[1] <- checks

    uint256 finalAmt = amount;
    if (unwrap) {
        //...
        } else if (tokenInfo.rebase == ETH_ADDRESS) {
            _unwrapETH(token, amount);
            (bool success,) = msg.sender.call{value: amount}(""); //[2] <- interaction
            if (!success) revert ETHSendFailed();

    //...
    unchecked {
        s.usersBalance[msg.sender][token] = balance - amount; //[3] <- effects
        s.tokens[token].totalStaked -= amount;
    }
    //...
}

Impact

An attacker could drain the WETH tokens available in the contract.

Recommendations

We recommend following the checks-effects-interactions pattern or adding a nonReentrant modifier to stop the reentrancy in this function.

Remediation

This issue has been acknowledged by Bracket Labs Group SA, and a fix was implemented in commit 6b7b4b1b↗.

Bracket Labs Group SA provided the following response:

Unchecking the balance subtraction was introduced while debugging an issue with rebase tokens and the development team forgot to remove it before the audit commit, and was originally meant to revert upon reentrancy. This was fixed immediately after the beginning of the audit, and nonReentrant modifier was added as an additional safeguard mechanism.