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

Bug in retireCarbonSpecific #25

Open
mahdiGhorbanzadeh opened this issue Aug 19, 2022 · 7 comments
Open

Bug in retireCarbonSpecific #25

mahdiGhorbanzadeh opened this issue Aug 19, 2022 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@mahdiGhorbanzadeh
Copy link

Hi,there is a bug in retireCarbonSpecific when poolToken and sourceToken is not equal and amountInCarbon is false

    function _prepareRetireSpecific(
        address _sourceToken,
        address _poolToken,
        uint256 _amount,
        bool _amountInCarbon,
        address _retiree
    ) internal returns (uint256, uint256) {
        (
            uint256 sourceAmount,
            uint256 totalCarbon,
            uint256 fee
        ) = _transferSourceTokens(
                _sourceToken,
                _poolToken,
                _amount,
                _amountInCarbon,
                true
            );

        // Get the pool tokens

        if (_sourceToken != _poolToken) {
            // Swap the source to get pool
            if (_amountInCarbon) {
                // Add redemption fee to the total to redeem.
                totalCarbon += _getSpecificCarbonFee(_poolToken, _amount);
                _amount += _getSpecificCarbonFee(_poolToken, _amount);

                // swapTokensForExactTokens
                _swapForExactCarbon(
                    _sourceToken,
                    _poolToken,
                    totalCarbon,
                    sourceAmount,
                    _retiree
                );
            } else {
                // swapExactTokensForTokens
                (_amount, fee) = _swapExactForCarbon(
                    _sourceToken,
                    _poolToken,
                    sourceAmount
                );
            }
        } else {
            _amount = sourceAmount - fee;
        }

        if (!_amountInCarbon) {
            // Calculate the fee and adjust if pool token is provided with false bool
            fee = (_amount * feeAmount) / 1000;
            _amount = totalCarbon - fee;
        }

        return (_amount, fee);
    }

example :

sourceToken : usdc
poolToken : bct
amountInCarbon:false
amount : 10 usdc

we consider 2 usdc equal to 1 bct

in this case _swapExactForCarbon return amount of bct (that is 4.95 bct if feeAmount is 1 %)

then because _amountInCarbon is false the below code also run:

if (!_amountInCarbon) {
    // Calculate the fee and adjust if pool token is provided with false bool
    fee = (_amount * feeAmount) / 1000;
    _amount = totalCarbon - fee;
}

and _amount updated here again with a value greater than the actual value that is 4.95

based on the calculations totalCarbon is 10 and _amount ‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍updated to something near to 9.9

in this case the above if must not be executed .

@0xAurelius
Copy link
Contributor

Reported by Discord user aliad010#0292

@0xAurelius 0xAurelius added the bug Something isn't working label Aug 19, 2022
@cujowolf
Copy link
Collaborator

@mahdiGhorbanzadeh thanks for the heads up! I'm not sure I follow where this bug is going though. Let's trace through the function call using your example values.

_sourceToken : usdc
_poolToken : bct
_amountInCarbon:false
_amount : 10 usdc (10000000 raw input)

A quick note on the values supplied to the reitreCarbon function call. If we were using the amountInCarbon as true and wanted to retire 5 tons, we would supply 5 (with appropriate decimals) for the amount field. You don't supply the amount including the fee (as it could change potentially)

            // Swap the source to get pool
            if (_amountInCarbon) {
                // Add redemption fee to the total to redeem.
                totalCarbon += _getSpecificCarbonFee(_poolToken, _amount);
                _amount += _getSpecificCarbonFee(_poolToken, _amount);

                // swapTokensForExactTokens
                _swapForExactCarbon(
                    _sourceToken,
                    _poolToken,
                    totalCarbon,
                    sourceAmount,
                    _retiree
                );
            } else {
                // swapExactTokensForTokens
                (_amount, fee) = _swapExactForCarbon(
                    _sourceToken,
                    _poolToken,
                    sourceAmount
                );
            }
        } else {
            _amount = sourceAmount - fee;
        }```

=== Swapping to the pool tokens ===
- We meet the first condition of this statement as USDC <> BCT.
- As the `amountInCarbon` is false, we execute the `swapExactTokensForTokens` function. This takes the total supplied token amount (10 USDC) and swaps all of it into the pool token. When this is done `_amount` is updated to be the value of the pool tokens received. Taking the value of BCT = 2 USDC, we `_amount` would now equal 5 (with decimals)

=== Adding in the fee for unspecified retirement amounts ===
- At this point we only know how many pool tokens we have been able to acquire, we need to calculate and return a fee.
I believe that the issue might lie within the `_amount = totalCarbon - fee;`, as `totalCarbon` in this case did not get properly updated along the way.

@aliad10
Copy link

aliad10 commented Aug 19, 2022

hi @cujowolf.

we agree that in our case this part execute

 (_amount, fee) = _swapExactForCarbon(
                    _sourceToken,
                    _poolToken,
                    sourceAmount
                );

and gives us 4.95 for _amount and 0.05 for the fee.
but the point is that

if (!_amountInCarbon) {
            // Calculate the fee and adjust if pool token is provided with false bool
            fee = (_amount * feeAmount) / 1000;
            _amount = totalCarbon - fee;
        }

also execute. and wrongly update the fee and _amount.

@aliad10
Copy link

aliad10 commented Aug 19, 2022

when

if (!_amountInCarbon) {
            // Calculate the fee and adjust if pool token is provided with false bool
            fee = (_amount * feeAmount) / 1000;
            _amount = totalCarbon - fee;
        }

executes and wrongly updates _amount (that is more than 4.95)
if we consider this part of code

        (_amount, fee) = _prepareRetireSpecific(
            _sourceToken,
            _poolToken,
            _amount,
            _amountInCarbon,
            _retiree
        );

in the retireToucanSpecific function we got wrong value for _amount and fee and try to retire that wrong value.
because we can retire only 4.95 bct but trying to retire something more than 4.95 bct execution must revert.

@cujowolf
Copy link
Collaborator

Ah yeah I see that now. Will get working on the fix here.

@aliad10
Copy link

aliad10 commented Aug 19, 2022

great 👍

@mahdiGhorbanzadeh
Copy link
Author

mahdiGhorbanzadeh commented Aug 19, 2022

You can do something like this (add return in else) @cujowolf

if (_amountInCarbon) {
                // Add redemption fee to the total to redeem.
                totalCarbon += _getSpecificCarbonFee(_poolToken, _amount);
                _amount += _getSpecificCarbonFee(_poolToken, _amount);

                // swapTokensForExactTokens
                _swapForExactCarbon(
                    _sourceToken,
                    _poolToken,
                    totalCarbon,
                    sourceAmount,
                    _retiree
                );
            } else {
                // swapExactTokensForTokens
                (_amount, fee) = _swapExactForCarbon(
                    _sourceToken,
                    _poolToken,
                    sourceAmount
                );
                return(_amount, fee);
            }

or change check like this

if (!_amountInCarbon && _sourceToken == _poolToken) {
            // Calculate the fee and adjust if pool token is provided with false bool
            fee = (_amount * feeAmount) / 1000;
            _amount = totalCarbon - fee;
        }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants