Users could run into DoS when calling collectRedemption() due to increased change of redemptionDelayBlocks

Summary

A potential vulnerability has been identified in the smart contract handling the redemption process of collateral in the Ubiquity Pool system. The issue arises from the ability of contract administrators to change poolStorage.redemptionDelayBlocks, potentially affecting users who have already initiated a redemption but have not yet collected their collateral.

Much as the admin is trusted, there isn't a way to prevent the incidents from happening unless poolStorage.unclaimedPoolCollateral[collateralIndex] ==0.

Vulnerability Detail

LibUbiquityPool.sol separates the redemption process into two functions: redeemDollar() and collectRedemption(). Users first call redeemDollar() to initiate the redemption, and after a delay defined by poolStorage.redemptionDelayBlocks, they call collectRedemption() to complete the process. However, if poolStorage.redemptionDelayBlocks is increased after a user has called redeemDollar() but before collectRedemption(), it could extend the waiting period unexpectedly, effectively causing a Denial of Service for the user.

Impact

The impact of this vulnerability is primarily on the user experience, potentially causing delays in the redemption process. It could erode trust in the system if users perceive that the delay period can be arbitrarily extended.

Code Snippet

https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L485-L492

        require(
            (
                poolStorage.lastRedeemedBlock[msg.sender].add(
                    poolStorage.redemptionDelayBlocks
                )
            ) <= block.number,
            "Too soon to collect redemption"
        );

https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L788-L796

    function setRedemptionDelayBlocks(
        uint256 newRedemptionDelayBlocks
    ) internal {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

        poolStorage.redemptionDelayBlocks = newRedemptionDelayBlocks;

        emit RedemptionDelayBlocksSet(newRedemptionDelayBlocks);
    }

Tool used

Manual Review

Recommendation

To mitigate this vulnerability, it is recommended to cache the poolStorage.redemptionDelayBlocks value at the time of calling redeemDollar(). This can be achieved by adding a new mapping in the UbiquityPoolStorage struct that records the redemption delay for each user at the time they initiate redemption. The collectRedemption() function should then refer to this user-specific delay rather than the global setting. This approach ensures that any changes to the global redemption delay do not affect users who have already initiated the redemption process.

Implementation example:

mapping(address => uint256) userRedemptionDelay;

function redeemDollar(...) internal ... {
    // ...
    poolStorage.userRedemptionDelay[msg.sender] = poolStorage.redemptionDelayBlocks;
    // ...
}

function collectRedemption(...) internal ... {
    require(
        poolStorage.lastRedeemedBlock[msg.sender].add(poolStorage.userRedemptionDelay[msg.sender]) <= block.number,
        "Too soon to collect redemption"
    );
    // ...
}

This change would safeguard the redemption process against unexpected delays, enhancing trust and reliability in the smart contract's operations.

sherlock-admin2 commented 6 months ago

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

It's protocol decision, and not affecting the protocol