search

code-423n4 / 2023-10-wildcat-findings

14 stars 10 forks source link

Tokens may still be stuck in the escrow contract even if the borrower overrides the sanction #382

Open c4-submissions opened 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsEscrow.sol#L38

Vulnerability details

Impact

Tokens will still be stuck in the escrow contract in same cases, even if the borrower overrides the sanction to release funds

Proof of Concept

Some accounts might be sanctioned due to legal reasons or any reason. If a lender is sanctioned, an escrow contract is created and the sanctioned lender's funds are transferred to this escrow contract. These funds can be released from the escrow contract after the sanction is removed or overridden.

The sanction status of an account is tracked with the ChainAnalysis sanction list. Additionally to that, the borrower has the power to override the sanction of an account. With this power, funds in the escrow can be released even if the account is still sanctioned at the oracle level.
Here is the overrideSanction function:
https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L48C1-L51C4

  function overrideSanction(address account) public override {
    sanctionOverrides[msg.sender][account] = true;
    emit SanctionOverride(msg.sender, account);
  }

Below, you can see the releaseEscrow() function:
https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsEscrow.sol#L33C1-L41C4

  function releaseEscrow() public override {
    if (!canReleaseEscrow()) revert CanNotReleaseEscrow();

    uint256 amount = balance();

    IERC20(asset).transfer(account, amount); //@audit it will only be transferred to the account. No other option. What if the account is still blocked at token contract level?

    emit EscrowReleased(account, asset, amount);
  }

If the sanction is overridden by the borrower, canReleaseEscrow() function will return true and assets will be transferred.

The issue here is the assets can only be transferred to the account itself.
Why is it a problem, you may ask?

The problem is that if an account is sanctioned in ChainAnalysis oracle, that account is very likely to be blocked at the contract level too for some tokens. For example, USDC has a blacklist option.
The aim of the sanctionOverride is being able to release funds even if the account is sanctioned. But if the account is blocked/blacklisted at the contract level, it will not matter if the borrower overrides the sanction or not. The funds can not be transferred regardless of the overriding, and the override function will not serve its purpose.

Tools Used

Manual review

Recommended Mitigation Steps

releaseEscrow() function is public, doesn't take any arguments, and transfers funds directly to the account. I think this one should remain still, and another function should be added.

The new function should only be called by the account itself, take an address argument, and transfer the funds to the provided address. This way, sanctionOverride functionality will serve its purpose even if the account itself is sanctioned at the contract level, and the account owner can transfer his/her funds to another address.

  function releaseEscrowToAnotherAddress(address _to) public override {
    if (!canReleaseEscrow()) revert CanNotReleaseEscrow();
->  if (msg.sender != account) revert YouAreNotTheOwnerOfTheFunds();

    uint256 amount = balance();

->  IERC20(asset).transfer(_to, amount);
    emit EscrowReleased(account, asset, amount);
  }

Assessed type

Token-Transfer

c4-pre-sort commented 1 year ago

minhquanym marked the issue as low quality report

minhquanym commented 1 year ago

Lender can transfer the market token to another address then withdraw

c4-judge commented 1 year ago

MarioPoneder marked the issue as unsatisfactory: Insufficient proof

osmanozdemir1 commented 1 year ago

Hi @MarioPoneder, Thanks for judging this contest.

I would like to share legal documents of two biggest stable coins to add some context. For USDC: Reference link Legal & Privacy - USDC Terms - Section 3. Applicable Laws and Regulations: "Applicable laws require us to prevent Restricted Persons from holding USDC using USDC Services. A Restricted Person means any person that is the subject or target of any sanctions, including a person that is:

For USDT: Reference link Terms of Service Clause 8.15: "...If Tether determines that you have engaged in any Prohibited Use, Tether may address such Prohibited Use through an appropriate sanction, in its sole and absolute discretion."

Clause 9: "...Tether in its absolute and sole discretion may determine that you are a customer of TIL or TLTD. Such determination will be communicated to you. Additionally, Tether monitors for and assesses suspicious or sanctionable transactions under applicable AML, Anti-Corruption, and Economic Sanctions Laws, as well as undertakes mandatory reporting to FinCEN, OFAC, FIA, and international regulators..."

These legal documents show that major stable token issuers track OFAC sanctions. A sanctioned account will most likely be blocked at the contract level too as mentioned in the original submission, which defeats the whole idea of sanction overriding. Tokens will still be locked in the escrow contract even if the borrower overrides the sanction. This line will pass after sanction overriding, but the transaction will revert a few lines below in this line.

Second thing I would like to mention is related to lookout's this comment:

Lender can transfer the market token to another address then withdraw

Unfortunately, this is not true. Tokens can only be released to the account itself. In fact, ability to transfer the market token to another address is exactly what I suggested as solution in the "recommended mitigation steps" section.

And last thing I would like to point out is in the contest page in C4 website (under the Attack Ideas (where to look for bugs)): "Consider ways in which parties to an escrow contract might be locked out of it, or the escrow contract might otherwise be bricked."

This submission shows how the escrow might be bricked and funds might still be locked even if the borrower overrides the sanctions. I would appreciate if you could reconsider the severity of this finding.

Kind regards

MarioPoneder commented 1 year ago

Thank you for your comment!

The "assets" in the escrow are actually the market tokens, not the underlying tokens. Therefore the transfer in releaseEscrow() cannot be blocked at token contract level.
The lender can just transfer the released market tokens to another address and then proceed to withdraw.

osmanozdemir1 commented 1 year ago

Hi again sir, thank you for the response.

I would like to point out that there might be two different escrow contracts for the same account, one for market tokens and the other one for underlying tokens.

Reference related to double escrow mechanism from the contest channel on discord.

If the escrows created in the executeWithdrawal() function, this line will create escrow for market tokens, and this will create escrow for underlying assets, which are transferred to escrow here.

Thanks again for your time

MarioPoneder commented 1 year ago

Thank you for this clarification! Given this input and the likelihood of impact while maintaining consistency with #717 and #305 (please review the reasoning in those issues), QA seems most appropriate.

c4-judge commented 1 year ago

MarioPoneder changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

MarioPoneder marked the issue as grade-b

osmanozdemir1 commented 1 year ago

Hi @MarioPoneder

Thank you for your response and referencing some issues.

I understand your reasoning in those referenced issues that says users decide which tokens to use and it's not a bug of protocol per se, and I agree with you for those cases.

However in this case, the protocol already has an additional functionality, which is overriding a sanction, and that functionality is broken.

This was my last comment and thanks for all of your efforts. Kind regards

MarioPoneder commented 1 year ago

Although it's not a bug of the protocol per se, the conflict between overriding the sanction status of the Chainalysis oracle while sactioning/blacklisting is still in place on token level is an extremely interesting thought.

c4-judge commented 1 year ago

MarioPoneder marked the issue as grade-a