feat: fix settleAndRefund vulnerability

[ChefMist]

Description

In #2 as we have balanceOf() and transfer(). We are prone to vulnerability when an ERC20 supports beforeTokenTransfer callback.

Example: assume $APE has beforeTokenTransfer and Alice is attacker

1/ Someone transfer 10 $APE to vault 
2/ Alice call settleAndRefund
3/ At the last part of settleAndRefund, vault transfer $APE trigger _beforeTokenTransfer
-> this is the part where we might get a call to `settleAndRefund` again 

REF: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v3.3/contracts/token/ERC20/ERC20.sol#L305

Fix

1/ instead of transfer, we mint the token to the address() instead. Then it'll be up to how the caller want to handle this minted token (can take 0 action or eventually call vault.burn() -> vault.take()

thx @ChefSnoopy and @chefburger for spotting this

[ChefMist]

before was 36514 now 34513 in cases wheres theres nothing to refund

[ChefMist]

before was 56176 in erc20 transfer, now 73023 in cases where there's excess token to refund

[jpopxfile]

Can cache currentDelta after casting to uint256 so you don't have to keep casting it again, wasting gas.

        if (currentDelta >= 0) {
            uint256 currentDeltaUint256 = currentDelta.toUint256();
            if (paid > currentDeltaUint256 ) {
                // msg.sender owes vault but paid more than whats owed
                refund = paid - currentDeltaUint256;
                paid = currentDeltaUint256;
            }
        }

[jpopxfile]

Also just to confirm, refund is only done, and paid is only adjusted if currentDelta is positive or zero, never if currentDelta is negative?

[ChefMist]

Also just to confirm, refund is only done, and paid is only adjusted if currentDelta is positive or zero, never if currentDelta is negative?

yep. do you think theres a scenario where negative balanceDelta (vault owes caller token) might need to handled?

[jpopxfile]

Not sure, that's why I'm asking.

[jpopxfile]

For this part, the function would revert if reservesBefore is larger than the current reserve, since it would mean that the locker did not transfer anything to increase the current balance right?

[ChefMist]

if the locker didn't transfer anything, this function shouldn't revert. The behavior should be the same as settle().

Can think of 1 scenario for revert:
1/ might be for rebasing token, where a negative rebase was done which cause the current balance in vault to be lesser than reserveBefore

cc @chefburger or @ChefSnoopy if can add on to more scenarios where revert might happen

[chefburger]

I cant come with other cases either, but i think it's fine to just let it revert if reservesBefore is greater than current.

[kritsadanuansutha]

---