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

Wrong token approval #99

Open
code423n4 opened this issue Jun 30, 2021 · 2 comments
Open

Wrong token approval #99

code423n4 opened this issue Jun 30, 2021 · 2 comments

Comments

@code423n4
Copy link
Contributor

Handle

cmichel

Vulnerability details

The pool holdings of Insurance (publicCollateralAmount and bufferCollateralAmount) is in WAD (18 decimals) but it's used as a raw token value in drainPool

// amount is a mix of pool holdings, i.e., 18 decimals
// this requires amount to be in RAW! if tracerMarginToken has > 18 decimals, it'll break, < 18 decimals will approve too much
tracerMarginToken.approve(address(tracer), amount);
// this requires amount to be in WAD which is correct
tracer.deposit(amount);

Impact

If tracerMarginToken has less than 18 decimals, the approval approves orders of magnitude more tokens than required for the deposit call that follows.
If tracerMarginToken has more than 18 decimals, the deposit that follows would fail as fewer tokens were approved, but the protocol seems to disallow tokens in general with more than 18 decimals.

Recommended Mitigation Steps

Convert the amount to a "raw token value" and approve this one instead.

@raymogg
Copy link
Collaborator

raymogg commented Jul 5, 2021

The issue is correct in pointing out that the wrong approve amount is used, however disagree with the severity.

It is common practice to approve the maximum amount of tokens for a contract to spend already. This bug simply allows more tokens to be approved (to a trusted contract in the system), than was intended. This is only exploitable if paired with another bug in the Tracer contracts. As is, no users would be affected.

@cemozerr
Copy link
Collaborator

Marking this as low-risk as it would only pose a security threat coupled with another bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants