c4-bot-8 commented 3 months ago

Lines of code

Vulnerability details

C4 issue

H-04: Withdrawals logic allows MEV exploits of TVL changes and zero-slippage zero-fee swaps

Comments

The current implementation of the WithdrawQueue contract follows a two-step withdrawal request and claim process with a set time in between. The issue is that the current implementation calculates and stores a user's withdrawal amount at the claim time, allowing users to instantly lock in a rate to withdraw. This opens up several attack vectors, such as MEV exploits and sandwich attacks. Furthermore, because the protocol's used tokens have the possibility of being slashed, users can front-run such actions to lock in their withdrawal at a better rate.

Mitigation

PR: Pull Request 111

This issue is mitigated by calculating the withdrawal amount at both the withdrawal request time and claim time, then taking the smaller of the two. This ensures that users cannot exploit rate changes to their advantage.

        // update withdraw request amount to redeem if lower at claim time.
        if (claimAmountToRedeem < _withdrawRequest.amountToRedeem) {
            _withdrawRequest.amountToRedeem = claimAmountToRedeem;
        }

Test

New test cases have been added to verify that the withdrawal amount is correctly calculated and compared at the claim time. All tests have passed, confirming the fix.

Contract: WithdrawQueueForkTest

Tests:

test_withdrawClaimMinAmount

Suggestion

An unhandled edge case could temporarily lock a significant portion of the protocol’s functionality. The getAvailableToWithdraw function, which is invoked when checking the buffer deficit, currently subtracts the contract's balance from the claimReserve. However, the claimReserve can sometimes exceed the contract's balance (previously acknowledged), such as through slashing or rebasing.

To prevent this, a check should be added to return zero if the claimReserve is greater than the contract balance, as shown below.

    function getAvailableToWithdraw(address _asset) public view returns (uint256) {
        if (_asset != IS_NATIVE) {
        if (claimReserve[_asset] > IERC20(_asset).balanceOf(address(this))) return 0; //ADD HERE
            return IERC20(_asset).balanceOf(address(this)) - claimReserve[_asset];
        } else {
        if (claimReserve[_asset] > address(this).balance) return 0; //ADD HERE
            return address(this).balance - claimReserve[_asset];
        }
    }

Conclusion

By calculating the withdrawal amount at both the request and claim times and using the smaller value, the mitigation effectively prevents MEV exploits and other attack vectors.

c4-judge commented 3 months ago

alcueca marked the issue as satisfactory