CreditAccountV1 Manual Review Findings

CAV-01M: Abnormal Credit Account Behaviour

Type	Severity	Location
Logical Fault		CreditAccountV1.sol:L14-L22, L35-L52

Description:

The current behaviour of CreditAccountV1 is significantly abnormal and contrasts its actual usage throughout the system (i.e. in VaultBase).

In detail, a particular owner can only retain one credit account at a time as evidenced by CreditAccountV1::getAccount. As such, it is possible to transfer one's own credit account to another user which will cause consequent invocations of CreditAccountV1::getAccount to utilize the transferred credit account rather than a fresh one for the user.

As a result, it is possible to f.e. create a position in the 0xPhase vault that is liquidated with a non-zero debt remaining and transfer it to a new unsuspecting user that wishes to enter the vault, causing them to deposit their funds to the existing, in-debt credit line that was transferred to them rather than to a fresh one.

Impact:

Any module that relies on a credit line and associates data with it is insecure as an on-chain race condition can manifest whereby a user is forced to accept a potentially low-health credit account.

Example:

Copy
10contract CreditAccountV1 is CreditAccountStorageV1 {
11  using CountersUpgradeable for CountersUpgradeable.Counter;
12
13  /// @inheritdoc ICreditAccount
14  function getAccount(address owner) external returns (uint256 tokenId) {
15    if (balanceOf(owner) > 0) return tokenOfOwnerByIndex(owner, 0);
16
17    _tokenIds.increment();
18    tokenId = _tokenIds.current();
19    _mint(owner, tokenId);
20
21    emit CreditAccountCreated(msg.sender, tokenId);
22  }
23
24  /// @inheritdoc ICreditAccount
25  function viewAccount(address owner) external view returns (uint256 tokenId) {
26    if (balanceOf(owner) == 0) return 0;
27    return tokenOfOwnerByIndex(owner, 0);
28  }
29
30  /// @inheritdoc ICreditAccount
31  function index() external view returns (uint256) {
32    return _tokenIds.current();
33  }
34
35  /// @inheritdoc CreditAccountStorageV1
36  function _beforeTokenTransfer(
37    address from,
38    address to,
39    uint256 tokenId,
40    uint256 batchSize
41  ) internal virtual override {
42    if (batchSize > 1) {
43      revert("CreditAccountV1: Consecutive transfers not supported");
44    }
45
46    require(
47      balanceOf(to) == 0,
48      "CreditAccountV1: Target already has a credit account"
49    );
50
51    super._beforeTokenTransfer(from, to, tokenId, batchSize);
52  }
53}

Recommendation:

We strongly advise CreditAccountV1 tokens to be soul-bound, disallowing their transfer as they should be bound per user over their lifetime. Alternatively, if a transfer of a CreditAccountV1 is desirable, we advise the contract to allow their transfers solely when they are activated by their intended recipient ensuring that a user can acquire another user's credit line only if they initiate the transaction rather than being transferred any credit line by any user.

Alleviation (3dd3d7bf0c):

The code of CreditAccountV1 was significantly refactored to introduce a pull approach to transfers whereby a user initiates a transfer and the recipient must accept it before actually receiving the credit line.

In the refactored implementation, we have observed the following issues / optimizations that should be fixed / applied:

• Subsequent invocations of CreditAccountV1::startTransfer should clear out the _transfersTo of the original recipient before setting it to the new to address.
• The CreditAccountV1::acceptTransfer flow should remove the from member and not the msg.sender when the info.token != token clause is evaluated as true.
• The structs declared should conform to VLF-01C
• The loop of CreditAccountV1::allTransfersTo should use unchecked code blocks to increment the iterator.

Once the issues of the latest implementation have been alleviated, we will mark this exhibit as properly dealt with.

Alleviation (19668501f8):

The code was updated according to our recommendation, however, the check was applied after the transfers[msg.sender] entry was overwritten rather than before it rendering it incorrect.

The currentTo != address(0) check must be evaluated before the transfers mapping entry is overwritten.

Alleviation (f921b6c4f9):

The code was re-ordered as per our recommendation, alleviating this exhibit in full and ensuring that the transfer entries of each user are correct.