Tendency - Deficient Implementation of redemptionDelayBlocks Requirement

[sherlock-admin]

Tendency

medium

Deficient Implementation of redemptionDelayBlocks Requirement

Summary

The current system allows calling UbiquityPoolFacet::collectRedemption a block before the set redemptionDelayBlocks, allowing users to claim their redeemed collateral earlier than the set delay

Vulnerability Detail

Ubiquity currently uses a two-step process of redeeming collateral for ubiquity dollars.
Firstly UbiquityPoolFacet::redeemDollar is called, after a set number of blocks has passed UbiquityPoolFacet::collectRedemption is then called to receive the redeemed collateral.

• The problem with the current implementation is this:

Assuming redemptionDelayBlocks is currently set to 5 blocks, which means after redeeming via a call to redeemDollar, collecting of this redeemed collateral is to be allowed after the set delay period of 5 blocks has elapsed, i.e. from the 6th block. The problem with the current system is that it wrongly allows the collection of redemption from the last block, i.e. from the 5th block, so from our example, users can still collect their redeemed collateral one block earlier than the intended delay.

I have added a runnable POC demonstrating the issue below:

Please add the below test to
packages/contracts/test/diamond/facets/UbiquityPoolFacet.t.sol, then run:

forge test -vvv --match-path test/diamond/facets/UbiquityPoolFacet.t.sol --match-test test_RedemptionDelay

    function test_RedemptionDelay() public {
        vm.startPrank(admin);
        ubiquityPoolFacet.setPriceThresholds(
            1000000, // mint threshold
            1000000 // redeem threshold
        );

        // set redemption delay to 5 blocks
        ubiquityPoolFacet.setRedemptionDelayBlocks(5);

        vm.stopPrank();

        // user sends 100 collateral tokens and gets 99 Dollars (-1% mint fee)
        vm.prank(user);
        ubiquityPoolFacet.mintDollar(
            0, // collateral index
            100e18, // Dollar amount
            99e18, // min amount of Dollars to mint
            100e18 // max collateral to send
        );

        // user redeems 99 Dollars for collateral
        vm.prank(user);
        ubiquityPoolFacet.redeemDollar(
            0, // collateral index
            99e18, // Dollar amount
            90e18 // min collateral out
        );

        //test that can still collect redemption one block prior

        // ideally this is supposed to wait at least 6 blocks before collecting redemption becomes active

        vm.roll(block.number + 5); //go five blocks further

        // balances before
        assertEq(collateralToken.balanceOf(address(ubiquityPoolFacet)), 100e18);
        assertEq(collateralToken.balanceOf(user), 0);

        vm.prank(user);
        uint256 collateralAmount = ubiquityPoolFacet.collectRedemption(0);
        assertEq(collateralAmount, 97.02e18); // $99 - 2% redemption fee
    }

Here are the logs:

[⠃] Compiling...
[⠆] Compiling 30 files with 0.8.19
[⠆] Solc 0.8.19 finished in 94.53s
Compiler run successful!

Running 1 test for test/diamond/facets/UbiquityPoolFacet.t.sol:UbiquityPoolFacetTest
[PASS] test_RedemptionDelay() (gas: 287692)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.00s
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

The admin set redemptionDelayBlocks wouldn't work as expected

Code Snippet

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

https://github.com/sherlock-audit/2023-12-ubiquity/blob/d9c39e8dfd5601e7e8db2e4b3390e7d8dff42a8e/ubiquity-dollar/packages/contracts/src/dollar/facets/UbiquityPoolFacet.sol#L107

https://github.com/sherlock-audit/2023-12-ubiquity/blob/d9c39e8dfd5601e7e8db2e4b3390e7d8dff42a8e/ubiquity-dollar/packages/contracts/src/dollar/facets/UbiquityPoolFacet.sol#L196-L199

Tool used

Manual Review

Recommendation

update collectRedemption check to:

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

This will ensure collection of redemption can only occur after the set wait period.

[sherlock-admin2]

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

auditsea commented:

It's protocol decision, and not affecting the protocol

[sherlock-admin2]

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

auditsea commented:

It's protocol decision, and not affecting the protocol

[nevillehuang]

Invalid, the logic is intended, and even if it is not, the maximum delay is 1 block.