sherlock-audit / 2023-04-splits-judging

4 stars 1 forks source link

Ace-30 - SappwerImpl: Malicious trader can drain out tokens because of difference in token decimals #84

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Ace-30

high

SappwerImpl: Malicious trader can drain out tokens because of difference in token decimals

Summary

there is no minimum amount for tokens to swap. swapping the minimum amount of base token can result in zero amount of quote token, especially when the decimals of quote token is less. So a malicious trader can call flash() and get some of the base tokens without sending any quote tokens

Vulnerability Detail

Suppose:

  1. Attacker calls flas() to get and swap 1 of token B and send token Q to the beneficiary
  2. transferToTrader, transfers 1 token B to the attacker
    • amountToTrader = baseAmount = 1 token B = 1e18 (in decimals)
    • qouteAmount = price baseAmount = 0.001 1 = 0.001 token Q = 0 (in decimals)
    • amountToBeneficiary = qouteAmount = 0
  3. attacker does not need to return any tokens, since amountToBeneficiary is 0 _transferToBeneficiary(_tokenToBeneficiary, amountToBeneficiary) tokenToBeneficiary_.safeTransferFrom(msg.sender, _beneficiary, amountToBeneficiary_);
  4. Attacker repeats the cycle to drain out all of token B in swapperImpl without paying any token

Note that since the tokenToBeneficiary (token Q) is 2 decimals, the amountToBeneficiary should be 2 decimals and can't be 18 decimals.

Impact

A malicious trader can drain out tokens without sending anything to the beneficiary

Code Snippet

https://github.com/0xSplits/splits-swapper/blob/83ce1124767a097aac37d1cd162a9b27bbc48701/src/SwapperImpl.sol#L203-L221

Tool used

Manual Review

Recommendation

revet or skip (continue) when the quoteAmount = 0