SavingsAccount.sol Wrong amount in Transfer events

[code423n4]

Handle

WatchPug

Vulnerability details

For the Transfer event, amount is expected to be the amount of tokens transferred, usually equal to the allowance decreased.

However, in the transfer() function, at L402: _amount is transformed into shares.

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/SavingsAccount/SavingsAccount.sol#L393-L416

function transfer(
    uint256 _amount,
    address _token,
    address _strategy,
    address _to
) external override returns (uint256) {
    require(_amount != 0, 'SavingsAccount::transfer zero amount');

    if (_strategy != address(0)) {
        _amount = IYield(_strategy).getSharesForTokens(_amount, _token);
    }

    balanceInShares[msg.sender][_token][_strategy] = balanceInShares[msg.sender][_token][_strategy].sub(
        _amount,
        'SavingsAccount::transfer insufficient funds'
    );

    //update receiver's balance
    balanceInShares[_to][_token][_strategy] = balanceInShares[_to][_token][_strategy].add(_amount);

    emit Transfer(_token, _strategy, msg.sender, _to, _amount);

    return _amount;
}

In the transferFrom() function, at L441: _amount is transformed into shares.

https://github.com/code-423n4/2021-12-sublime/blob/9df1b7c4247f8631647c7627a8da9bdc16db8b11/contracts/SavingsAccount/SavingsAccount.sol#L426-L456

function transferFrom(
    uint256 _amount,
    address _token,
    address _strategy,
    address _from,
    address _to
) external override returns (uint256) {
    require(_amount != 0, 'SavingsAccount::transferFrom zero amount');
    //update allowance
    allowance[_from][_token][msg.sender] = allowance[_from][_token][msg.sender].sub(
        _amount,
        'SavingsAccount::transferFrom allowance limit exceeding'
    );

    if (_strategy != address(0)) {
        _amount = IYield(_strategy).getSharesForTokens(_amount, _token);
    }

    //reduce sender's balance
    balanceInShares[_from][_token][_strategy] = balanceInShares[_from][_token][_strategy].sub(
        _amount,
        'SavingsAccount::transferFrom insufficient allowance'
    );

    //update receiver's balance
    balanceInShares[_to][_token][_strategy] = (balanceInShares[_to][_token][_strategy]).add(_amount);

    emit Transfer(_token, _strategy, _from, _to, _amount);

    return _amount;
}

As a result, the amount in Transfer events is wrong.

PoC

Given:

• the price per share of yearn USDC vault is 1.2

1. Alice deposited 12,000 USDC to yearn strategy, received 10,000 share tokens;
2. Alice transfer() 12,000 USDC to Bob;

• Expected Result: the amount in the Transfer event is: 12,000;
• Actual Result: the amount in the Transfer event is: 10,000.

[ritik99]

Since the function internally deals with shares rather than the underlying tokens the implementation is actually correct. We'll make it more explicit in the event definition. Since the issue deals with off-chain monitoring we suggest a rating of (0) Non-critical

[0xean]

0 — Non-critical: Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas-optimisations.