thisvishalsingh - Inadequate Access Control in `unlock` Function Leading to Griefing and State Bloat

[sherlock-admin4]

thisvishalsingh

medium

Inadequate Access Control in unlock Function Leading to Griefing and State Bloat

Summary

The DelayedWETH contract unlock function permits any user to initiate an unlock request, which could lead to misunderstandings and operational inefficiencies. This function does not limit unlock requests to the fund owners themselves, potentially allowing for unauthorized state changes and confusion among users.

Vulnerability Detail

The unlock function is designed to allow any address to request an unlock, which then records a new timestamp and modifies the withdrawal amount in the withdrawals mapping. This mapping is indexed by the caller (msg.sender) and the specified recipient (_guy). The protocol team believes this design is secure under proper usage but acknowledges the possibility of making it safer, albeit at the cost of added complexity.

 /// @inheritdoc IDelayedWETH
    function unlock(address _guy, uint256 _wad) external {
        // Note that the unlock function can be called by any address, but the actual unlocking capability still only
        // gives the msg.sender the ability to withdraw from the account. As long as the unlock and withdraw functions
        // are called with the proper recipient addresses, this will be safe. Could be made safer by having external
        // accounts execute withdrawals themselves but that would have added extra complexity and made DelayedWETH a
        // leaky abstraction, so we chose this instead.
        WithdrawalRequest storage wd = withdrawals[msg.sender][_guy];
        wd.timestamp = block.timestamp;
        wd.amount += _wad;
    }

The withdraw function, which is designed to process withdrawals based on the withdrawals mapping, contains several checks to ensure the legitimacy of a withdrawal attempt. It verifies that the contract is not paused, the withdrawal amount is available and unlocked, the unlock request has been timestamped, and the required delay period has elapsed.

 /// @inheritdoc IDelayedWETH
    function withdraw(address _guy, uint256 _wad) public {
        require(!config.paused(), "DelayedWETH: contract is paused");
        WithdrawalRequest storage wd = withdrawals[msg.sender][_guy];
        require(wd.amount >= _wad, "DelayedWETH: insufficient unlocked withdrawal");
        require(wd.timestamp > 0, "DelayedWETH: withdrawal not unlocked");
        require(wd.timestamp + DELAY_SECONDS <= block.timestamp, "DelayedWETH: withdrawal delay not met");
        wd.amount -= _wad;
        super.withdraw(_wad);
    }

Although these checks protect against unauthorized withdrawals, the open nature of the unlock function could still be exploited for griefing. An attacker could generate numerous unlock requests, leading to a bloated contract state and potentially higher gas costs. Additionally, if user interfaces or automated systems display unlock information based on the withdrawals mapping, they might show misleading data, eroding user trust and complicating interactions.

Hence, there is no direct financial risk due to unauthorized withdrawals, the open design of the unlock function could be used to create confusion or inflate the contract's state with unnecessary unlock requests, potentially leading to operational inefficiencies.

Risks:

1. There is a risk of attackers creating confusion by initiating unauthorized unlock requests.
2. The contract's state could be unnecessarily increased, leading to higher gas costs for users.
3. Users may experience confusion if they encounter unlock requests that they did not initiate.

Impact

The primary impact of this design choice is on the user experience and operational costs. While the withdraw function contains checks that safeguard against unauthorized fund transfers, the potential for confusion and state bloat remains. Users can be misled by seeing unlock requests that they did not authorize, potentially eroding trust in the contract's security model. Additionally, the contract could face increased storage costs due to the accumulation of unlock requests, which could translate into higher gas fees for users.

Code Snippet

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/f216b0d3ad08c1a0ead557ea74691aaefd5fd489/optimism/packages/contracts-bedrock/src/dispute/weth/DelayedWETH.sol#L57

 function unlock(address _guy, uint256 _wad) external {
        WithdrawalRequest storage wd = withdrawals[msg.sender][_guy];
        wd.timestamp = block.timestamp;
        wd.amount += _wad;
    }

Tool used

Manual Review

Recommendation

• consider implementing optional, user-driven safeguards for those who prefer stricter access control over their funds. This could include features that allow users to opt-in to additional security checks or to set permissions for which addresses can initiate unlock requests on their behalf.

[nevillehuang]

Invalid, one withdrawer spamming unlock requests is not going to affect another withdrawer since withdrawals are executed from the context of the caller.