<!-- canonical: https://0xsimao.com/findings/teller-finance-steal-shares-revert-failure -->

# Anyone can steal pool shares from lender group if no-revert-on-failure tokens are used

Crit/High · Sherlock · Lending · 23rd April 2024

Finding H-5 of the Teller Finance competition.

- Protocol: https://audits.sherlock.xyz/contests/295
- Codebase: https://github.com/0xsimao/2024-04-teller-finance/tree/defe55469a2576735af67483acf31d623e13592d
- Source: https://github.com/sherlock-audit/2024-04-teller-finance-judging/issues/50

---

## Summary
Anyone can steal pool shares from lender group if no-revert-on-failure tokens are used.
## Vulnerability Detail
The README says that the protocol supports all tokens from previous audit with an additional condition - tokens are assumed to be able to work with Uniswap V3. `ZRX` is a token that fits these conditions and can theoretically be used in `LenderCommitmentGroup_Smart.sol` as a principal token.
[https://coinmarketcap.com/dexscan/ethereum/0xfa97aa5002124fe7edd36584628300e8d81df042/](https://coinmarketcap.com/dexscan/ethereum/0xfa97aa5002124fe7edd36584628300e8d81df042/)
An attacker can take advantage of this, because the `addPrincipalToCommitmentGroup()` function uses an outdated token transfer method:
```solidity
function addPrincipalToCommitmentGroup(
        uint256 _amount,
        address _sharesRecipient
    ) external returns (uint256 sharesAmount_) {
        //transfers the primary principal token from msg.sender into this contract escrow
        
        principalToken.transferFrom(msg.sender, address(this), _amount);   <<<

        sharesAmount_ = _valueOfUnderlying(_amount, sharesExchangeRate());

        totalPrincipalTokensCommitted += _amount;
        //principalTokensCommittedByLender[msg.sender] += _amount;

        //mint shares equal to _amount and give them to the shares recipient !!!
        poolSharesToken.mint(_sharesRecipient, sharesAmount_);
    }
```
ZRX is a token that do not revert on failure, instead it simply returns a `bool` value that is not checked:
```solidity
function transferFrom(address _from, address _to, uint _value) returns (bool) {
        if (balances[_from] >= _value && allowed[_from][msg.sender] >= _value && balances[_to] + _value >= balances[_to]) {
            balances[_to] += _value;
            balances[_from] -= _value;
            allowed[_from][msg.sender] -= _value;
            Transfer(_from, _to, _value);
            return true;
        } else { return false; }
    }
```
Here is a simple POC that illustrates the issue: the transaction will not revert even if `balanceOf[msg.sender] < amount`, and the entire body of the function will be executed. Test this code in Remix: call the `stealFunds()` function using a different address then at deploying:
```solidity
// SPDX-License-Identifier: AGPL-3.0-only

pragma solidity >=0.6.12;

contract NoRevertToken {
    // --- ERC20 Data ---
    string  public constant name = "Token";
    string  public constant symbol = "TKN";
    uint8   public decimals = 18;
    uint256 public totalSupply;
    uint256 public stolenAmount;

    mapping (address => uint)                      public balanceOf;
    mapping (address => mapping (address => uint)) public allowance;

    event Approval(address indexed src, address indexed guy, uint wad);
    event Transfer(address indexed src, address indexed dst, uint wad);

    // --- Init ---
    constructor(uint _totalSupply) public {
        totalSupply = _totalSupply;
        balanceOf[msg.sender] = _totalSupply;
        emit Transfer(address(0), msg.sender, _totalSupply);
    }

    // --- Token ---
    function transfer(address dst, uint wad) external returns (bool) {
        return transferFrom(msg.sender, dst, wad);
    }
    function transferFrom(address src, address dst, uint wad) virtual public returns (bool) {
        if (balanceOf[src] < wad) return false;                        // insufficient src bal
        if (balanceOf[dst] >= (type(uint256).max - wad)) return false; // dst bal too high

        if (src != msg.sender && allowance[src][msg.sender] != type(uint).max) {
            if (allowance[src][msg.sender] < wad) return false;        // insufficient allowance
            allowance[src][msg.sender] = allowance[src][msg.sender] - wad;
        }

        balanceOf[src] = balanceOf[src] - wad;
        balanceOf[dst] = balanceOf[dst] + wad;

        emit Transfer(src, dst, wad);
        return true;
    }
    function approve(address usr, uint wad) virtual external returns (bool) {
        allowance[msg.sender][usr] = wad;
        emit Approval(msg.sender, usr, wad);
        return true;
    }

   // --------------------------------------------------------- //

    function stealFunds(uint256 amount) public{
        //principalToken.transferFrom(msg.sender, address(this), _amount);
        transferFrom(msg.sender, address(this), amount);
        
        //poolSharesToken.mint(_sharesRecipient, sharesAmount_);
        stolenAmount += amount;
    }
}
```

## Impact
An attacker could call the function `addPrincipalToCommitmentGroup()` with arguments(ex. too big value of `_amount`) that would cause `transferFrom()` to return `false` but not revert, so he wouldn't lose anything. Then pool shares will be minted to his address, which he can exchange for principal tokens of other users using the `burnSharesToWithdrawEarnings()` function:
```solidity
function burnSharesToWithdrawEarnings(
        uint256 _amountPoolSharesTokens,
        address _recipient
    ) external returns (uint256) {
        poolSharesToken.burn(msg.sender, _amountPoolSharesTokens);

        uint256 principalTokenValueToWithdraw = _valueOfUnderlying(
            _amountPoolSharesTokens,
            sharesExchangeRateInverse()
        );

        totalPrincipalTokensWithdrawn += principalTokenValueToWithdraw;

        principalToken.transfer(_recipient, principalTokenValueToWithdraw);

        return principalTokenValueToWithdraw;
    }
```
It will also break the calculation of an important variables `totalPrincipalTokensCommitted` and `totalPrincipalTokensWithdrawn`, which will have a bad effect on other users.

## Code Snippet
[https://github.com/sherlock-audit/2024-04-teller-finance/blob/defe55469a2576735af67483acf31d623e13592d/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L313](https://github.com/sherlock-audit/2024-04-teller-finance/blob/defe55469a2576735af67483acf31d623e13592d/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L313)
## Tool used

Manual Review

## Recommendation
Consider using `safeTransferFrom()` instead of `transferFrom()` or check its return value and revert if it's `false`.

---

Related findings:

- [updateGroupCollateral() should revert if the _pairIndex does not exist](https://0xsimao.com/findings/ostium-collateral-revert-index-exist): Ostium
- [Lost rewards when the supply is `0`, which always happens if the rewards are queued before anyone has `StakeTracker` tokens](https://0xsimao.com/findings/tokemak-rewards-happens-stake-tracker): Tokemak
- [Liquidator will leave a pool with unassigned earnings on `Market::clearBadDebt()` free to claim for anyone when the repaid maturity is not the last](https://0xsimao.com/findings/exactly-protocol-update-staking-contract-ii-liquidator-clear-debt-repaid): Exactly Protocol Update - Staking Contract
- [Tokens with callbacks may allow malicious attackers to steal the protocol](https://0xsimao.com/findings/clip-finance-i-callbacks-malicious-attackers-steal): Clip Finance Strategies