# Security Review of Tether Join

## Review Summary
**No critical or high impact issues were found.**
The contract has a very small surface of attack as the state changing external functions are protected by access control with the exception of `flashLoan`.
There is only one state changing external call which is to USDT contract for transferring funds.
## Scope
The review is based on the commit [99464fe](https://github.com/yieldprotocol/vault-v2/pull/518/commits/99464feda7547bce49b84cb6b01d988d517a9818). TetherJoin.sol was the primary focus of this audit. The relevant parts of the inherited contracts `FlashJoin.sol` & `Join.sol` were also reviewed.
The goal of the review was to identify if there are any vulnerabilities or any discrepancies in the calculations that would result in unintended outcomes.
## Note on developer response
The findings were acknowledged. The design was not changed and comments were added to make the caveats of the design choices clear to the users building on top of the join.
## Findings
There was only one low impact issue which is dependent on change in state of USDT.
### **Activation of fees on Tether**
Tether's ERC20 contract has an implementation for charging fees whenever it is transferred. At the time of the audit it was not activated. However, when it gets activated it could lead to problems in the protocol.

**1. Exit returns incorrect `amount` of asset transferred to `user` (LOW)**
If `Tether` starts charging fees for transfer the amount returned by `exit` would reflect incorrect amount. This would happen as the tether would charge a fee and as a result the amount transferred to the `user` would be less. The return value will lead to issues if used in business logic.
#### Recommendation
Implement changes in the `_exit` function such that it returns the correct value.
[Current Implementation](https://github.com/yieldprotocol/vault-v2/blob/99464feda7547bce49b84cb6b01d988d517a9818/packages/foundry/contracts/Join.sol#L48-L54)

/// @dev Transfer `amount` `asset` to `user`
function _exit(address user, uint128 amount) internal virtual returns (uint128) {
    IERC20 token = IERC20(asset);
    storedBalance -= amount;
    token.safeTransfer(user, amount);
    return amount;
}

Suggested Implementation

function _exit(address user, uint128 amount) internal override returns (uint128) {
    IERC20 token = IERC20(asset);
    storedBalance -= amount;
    token.safeTransfer(user, amount);
    amount -= uint128(
        _min(
            (amount * IUSDT(asset).basisPointsRate()) / 10000,
            IUSDT(asset).maximumFee()
        )
    );
    return amount;
}

#### Developer Response
>We decided that the TetherJoin would take a selfish approach, and that the same would be done by every other contract in the protocol.
This selfish approach means that the TetherJoin will always expect to receive exactly the token amounts in function arguments. TetherJoin will not care about the token amounts sent to it.
With this logic, when calling `join(from, x)`, TetherJoin will always receive x USDT, therefore the reverse fee calculation was implemented for the cases in which the TetherJoin pulls tokens from the caller.
Following this logic, when calling `exit(to, x)`, TetherJoin will always send x tokens towards the receiver, and it will be the responsibility of the receiver to make sure it receives the amount intended.
In most cases, this would mean implementing the reverse fee formula themselves, as we did.
#### Reviewer response:
Change reviewed. No issues noted.

**2. Flash Loan (Informational)**
If fees are activated then the users would end up receiving less amount of loan then they requested for due to deduction of fees during the transfer. If users don't take this into account they might end up with broken logic in their contract due to incorrect assumptions.
#### Recommendation
1. Modify `flashLoan` functionality such that it reports correct numbers
2. Add descriptive comments in the code to make user aware of the potential scenario
#### Developer Response
> As before, the TetherJoin will enforce that it sends what is it instructed to send, and that it receives what it is told it should be receiving.
> The flash loan user should be aware that if there are fees applied, it will need to reverse the fee to receive the amount of USDT intended. Likewise, when repaying the flash loan, the TetherJoin will pull from the flash loan receiver the loaned amount plus the USDT fee, plus a flash loan fee if applicable.
#### Reviewer response:
Change reviewed. No issues noted.