search

code-423n4 / 2021-10-tracer-findings

0 stars 0 forks source link

`commit` burn yields wrong `amountOut` computation #18

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

cmichel

Vulnerability details

The PoolCommitter.commit function first adds the amount to the shadow pool (shadowPools[_commitType] = shadowPools[_commitType] + amount) and then computes the amountOut with this updated value already:

PoolSwapLibrary.getWithdrawAmountOnBurn(
    IERC20(tokens[1]).totalSupply(),
    amount,
    shortBalance,
    // this includes the updated amount already
    shadowPools[_commitType]
)

The shadow pool must be updated only after the computation.

Impact

It leads to a wrong amountOut and commitments that should be accepted could be denied.

Example: Initial values: totalSupply = 1000, longBalance = 1000, shadowPools[LongBurn] = 0. The formula for amountOut is: balance * amountIn / (tokenSupply + shadowBalance)

  1. Single deposit case - amount = 1000: Note how shadowPools[LongBurn] is updated to 1000 first. Then, amountOut = 1000 * 1000 / (1000 + 1000) = 1000 * 1/2 = 500. Clearly if we intend to burn all existing tokens, the totalSupply, we should receive the entire balance, but we only receive half of it.

This under-reports the actual amount we receive and can therefore lead to legitimate burns being dismissed due to the require(amountOut >= minimumCommitSize) check.

The actual amountOut that the user receives in executeCommitment is correct, the issue is just in commit.

The same issue exists for the ShortBurn case.

Recommended Mitigation Steps

Increase the shadow pool after the computation. It makes sense to do it after the actual burn call as the invariant is that totalSupply_long + shadowPools[LongBurn] should stay constant across burn commits.

kumar-ish commented 3 years ago

This issue is valid; however, as the first point in the known issues in the contracts in the C4 repo for this contest, this issue was marked out of scope: https://github.com/code-423n4/2021-10-tracer#c4-audit-known-issues

GalloDaSballo commented 3 years ago

The finding was excluded, I've checked that the readme wasn't changed during the contest

As such the issue is invalid