Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[INFORMATIONAL-1] Allowing Zero-Value Recovery (Lack of Amount Validation) #119

Open
hats-bug-reporter bot opened this issue Nov 12, 2024 · 1 comment
Labels
invalid This doesn't seem right

Comments

@hats-bug-reporter
Copy link

Github username: @codertjay
Twitter username: codertjay
Submission hash (on-chain): 0x2d8b300f1bbeb5ec78ff4ff85342a08df891f299a34cf9ac17ed993374872331
Severity: low

Description:

[INFORMATIONAL-1] Allowing Zero-Value Recovery (Lack of Amount Validation)

https://github.com/eurodollar-fi/eurodollar-protocol/blob/3900ae6a01f5c60146d314bf45b2ab67179422d1/src/USDE.sol#L151

Description:
The recover function allows a zero-value amount to be passed as the recovery amount, which triggers an unnecessary _burn
and _mint operation, emits a Recovered event, and incurs unnecessary transaction costs. This occurs because there is no
validation in place to ensure the amount is greater than zero before executing the function’s core logic. This issue
could also lead to log cluttering from irrelevant zero-amount recovery events and unnecessary usage of gas resources.

Impact:

Allows zero-value token recovery transactions, resulting in unnecessary gas costs, event clutter in logs, and potential
confusion over legitimate versus zero-value transactions.
Adds the risk of performance inefficiencies, especially if exploited in bulk by malicious actors.
Reduces overall readability and reliability of emitted Recovered events, affecting data indexing and auditing of actual
token recoveries.

Proof of Concept:
Here’s how the issue occurs in the current code structure:

// Calling recover with zero amount

function test_mint_rec_fuzz(address to,address from , uint256 amount) 
      public {
      vm.assume(to != address(0));
      vm.assume(amount == 0);
    
      vm.prank(rescue_user);
      usde.recover(from, to, 0);

}

Ran 1 test for test/USDE.t.sol:USDETest
[PASS] test_mint_rec_fuzz(address,address,uint256) (runs: 501, μ: 46340, ~: 46340)
Traces:
[46340] USDETest::test_mint_rec_fuzz(0xBd5E91d94421846Ab96cCd5cE60166383e6503F5, 0x0966cF3AaA340Da069Dc3cAC0f5Fa689fFEb6246, 0)
├─ [0] VM::assume(true) [staticcall]
│   └─ ← [Return]
├─ [0] VM::assume(true) [staticcall]
│   └─ ← [Return]
├─ [0] VM::prank(0x0000000000000000000000000000000000000000)
│   └─ ← [Return]
├─ [35049] ERC1967Proxy::recover(0x0966cF3AaA340Da069Dc3cAC0f5Fa689fFEb6246, 0xBd5E91d94421846Ab96cCd5cE60166383e6503F5, 0)
│   ├─ [30156] USDE::recover(0x0966cF3AaA340Da069Dc3cAC0f5Fa689fFEb6246, 0xBd5E91d94421846Ab96cCd5cE60166383e6503F5, 0) [delegatecall]
│   │   ├─ [5016] Validator::isValid(0x0966cF3AaA340Da069Dc3cAC0f5Fa689fFEb6246, 0x0000000000000000000000000000000000000000) [staticcall]
│   │   │   └─ ← [Return] true
│   │   ├─ emit Transfer(from: 0x0966cF3AaA340Da069Dc3cAC0f5Fa689fFEb6246, to: 0x0000000000000000000000000000000000000000, value: 0)
│   │   ├─ [3016] Validator::isValid(0x0000000000000000000000000000000000000000, 0xBd5E91d94421846Ab96cCd5cE60166383e6503F5) [staticcall]
│   │   │   └─ ← [Return] true
│   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0xBd5E91d94421846Ab96cCd5cE60166383e6503F5, value: 0)
│   │   ├─ emit Recovered(from: 0x0966cF3AaA340Da069Dc3cAC0f5Fa689fFEb6246, to: 0xBd5E91d94421846Ab96cCd5cE60166383e6503F5, amount: 0)
│   │   └─ ← [Return] true
│   └─ ← [Return] true
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 801.83ms (792.27ms CPU time)
Ran 1 test suite in 1.74s (801.83ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

This call would:

Trigger _burn and _mint operations with zero-value input, which consumes gas.
Emit the Recovered event despite no meaningful token transfer occurring.
Recommended Mitigation:

Add a validation check to ensure the amount is greater than zero before proceeding with the recovery:

require(amount > 0, "Recovery amount must be greater than zero");

Optionally consider implementing a pause mechanism for critical functions like recover to help mitigate misuse in
emergency scenarios.

@hats-bug-reporter hats-bug-reporter bot added the bug Something isn't working label Nov 12, 2024
@AndreiMVP
Copy link

Not an issue, it's out of scope:

https://app.hats.finance/audit-competitions/euro-dollar-0xa4ccd3b6daa763f729ad59eae75f9cbff7baf2cd/scope

  1. Issues where the user is harming itself by interacting improperly with the contracts. It's possible for the users to call functions with improper parameters, incompatible tokens, etc but we assume that functions are called properly similarly to how they are called in the frontend.

@geaxed geaxed added invalid This doesn't seem right and removed bug Something isn't working labels Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

2 participants