Submission Details
Severity:
FoT tokens like PAXG won’t work seamlessly with the protocol
Summary
In the About the project section we can read that the protocol aims to Secure your crypto assets, such as ETH, WBTC, ARB, LINK, & PAXG tokenized gold, in smart contracts that you control and no one else. One of the mentioned tokens, PAXG, is according to their website charging a 0.02% on-chain transaction fee that is covered by the users.
This leads to a couple of problems related to the effective collateral rate and Uniswap V3 integrations.
Vulnerability Details
Design and testing problem
The first step in checking if the protocol was designed with FoT token usage in mind was to change the ERC20Mock.sol file with the following:
// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v4.9.0) (token/ERC20/ERC20.sol)
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
/**
* @dev Implementation of the {IERC20} interface.
*
* This implementation is agnostic to the way tokens are created. This means
* that a supply mechanism has to be added in a derived contract using {_mint}.
* For a generic mechanism see {ERC20PresetMinterPauser}.
*
* TIP: For a detailed writeup see our guide
* https://forum.openzeppelin.com/t/how-to-implement-erc20-supply-mechanisms/226[How
* to implement supply mechanisms].
*
* The default value of {decimals} is 18. To change this, you should override
* this function so it returns a different value.
*
* We have followed general OpenZeppelin Contracts guidelines: functions revert
* instead returning `false` on failure. This behavior is nonetheless
* conventional and does not conflict with the expectations of ERC20
* applications.
*
* Additionally, an {Approval} event is emitted on calls to {transferFrom}.
* This allows applications to reconstruct the allowance for all accounts just
* by listening to said events. Other implementations of the EIP may not emit
* these events, as it isn't required by the specification.
*
* Finally, the non-standard {decreaseAllowance} and {increaseAllowance}
* functions have been added to mitigate the well-known issues around setting
* allowances. See {IERC20-approve}.
*/
abstract contract Context {
function _msgSender() internal view virtual returns (address) {
return msg.sender;
}
function _msgData() internal view virtual returns (bytes calldata) {
return msg.data;
}
function _contextSuffixLength() internal view virtual returns (uint256) {
return 0;
}
}
contract ERC20Mock is Context, IERC20, IERC20Metadata {
mapping(address => uint256) private _balances;
mapping(address => mapping(address => uint256)) private _allowances;
uint256 private _totalSupply;
string private _name;
string private _symbol;
uint8 private _decimals;
/**
* @dev Sets the values for {name} and {symbol}.
*
* All two of these values are immutable: they can only be set once during
* construction.
*/
constructor(string memory name_, string memory symbol_, uint8 decimals) {
_name = name_;
_symbol = symbol_;
_decimals = decimals;
}
/**
* @dev Returns the name of the token.
*/
function name() public view virtual override returns (string memory) {
return _name;
}
/**
* @dev Returns the symbol of the token, usually a shorter version of the
* name.
*/
function symbol() public view virtual override returns (string memory) {
return _symbol;
}
/**
* @dev Returns the number of decimals used to get its user representation.
* For example, if `decimals` equals `2`, a balance of `505` tokens should
* be displayed to a user as `5.05` (`505 / 10 ** 2`).
*
* Tokens usually opt for a value of 18, imitating the relationship between
* Ether and Wei. This is the default value returned by this function, unless
* it's overridden.
*
* NOTE: This information is only used for _display_ purposes: it in
* no way affects any of the arithmetic of the contract, including
* {IERC20-balanceOf} and {IERC20-transfer}.
*/
function decimals() public view virtual override returns (uint8) {
return _decimals;
}
/**
* @dev See {IERC20-totalSupply}.
*/
function totalSupply() public view virtual override returns (uint256) {
return _totalSupply;
}
/**
* @dev See {IERC20-balanceOf}.
*/
function balanceOf(address account) public view virtual override returns (uint256) {
return _balances[account];
}
/**
* @dev See {IERC20-transfer}.
*
* Requirements:
*
* - `to` cannot be the zero address.
* - the caller must have a balance of at least `amount`.
*/
function transfer(address to, uint256 amount) public virtual override returns (bool) {
address owner = _msgSender();
_transfer(owner, to, amount);
return true;
}
/**
* @dev See {IERC20-allowance}.
*/
function allowance(address owner, address spender) public view virtual override returns (uint256) {
return _allowances[owner][spender];
}
/**
* @dev See {IERC20-approve}.
*
* NOTE: If `amount` is the maximum `uint256`, the allowance is not updated on
* `transferFrom`. This is semantically equivalent to an infinite approval.
*
* Requirements:
*
* - `spender` cannot be the zero address.
*/
function approve(address spender, uint256 amount) public virtual override returns (bool) {
address owner = _msgSender();
_approve(owner, spender, amount);
return true;
}
/**
* @dev See {IERC20-transferFrom}.
*
* Emits an {Approval} event indicating the updated allowance. This is not
* required by the EIP. See the note at the beginning of {ERC20}.
*
* NOTE: Does not update the allowance if the current allowance
* is the maximum `uint256`.
*
* Requirements:
*
* - `from` and `to` cannot be the zero address.
* - `from` must have a balance of at least `amount`.
* - the caller must have allowance for ``from``'s tokens of at least
* `amount`.
*/
function transferFrom(address from, address to, uint256 amount) public virtual override returns (bool) {
address spender = _msgSender();
_spendAllowance(from, spender, amount);
_transfer(from, to, amount);
return true;
}
/**
* @dev Atomically increases the allowance granted to `spender` by the caller.
*
* This is an alternative to {approve} that can be used as a mitigation for
* problems described in {IERC20-approve}.
*
* Emits an {Approval} event indicating the updated allowance.
*
* Requirements:
*
* - `spender` cannot be the zero address.
*/
function increaseAllowance(address spender, uint256 addedValue) public virtual returns (bool) {
address owner = _msgSender();
_approve(owner, spender, allowance(owner, spender) + addedValue);
return true;
}
/**
* @dev Atomically decreases the allowance granted to `spender` by the caller.
*
* This is an alternative to {approve} that can be used as a mitigation for
* problems described in {IERC20-approve}.
*
* Emits an {Approval} event indicating the updated allowance.
*
* Requirements:
*
* - `spender` cannot be the zero address.
* - `spender` must have allowance for the caller of at least
* `subtractedValue`.
*/
function decreaseAllowance(address spender, uint256 subtractedValue) public virtual returns (bool) {
address owner = _msgSender();
uint256 currentAllowance = allowance(owner, spender);
require(currentAllowance >= subtractedValue, "ERC20: decreased allowance below zero");
unchecked {
_approve(owner, spender, currentAllowance - subtractedValue);
}
return true;
}
/**
* @dev Moves `amount` of tokens from `from` to `to`.
*
* This internal function is equivalent to {transfer}, and can be used to
* e.g. implement automatic token fees, slashing mechanisms, etc.
*
* Emits a {Transfer} event.
*
* Requirements:
*
* - `from` cannot be the zero address.
* - `to` cannot be the zero address.
* - `from` must have a balance of at least `amount`.
*/
function _transfer(address from, address to, uint256 amount) internal virtual {
require(from != address(0), "ERC20: transfer from the zero address");
require(to != address(0), "ERC20: transfer to the zero address");
_beforeTokenTransfer(from, to, amount);
uint256 fromBalance = _balances[from];
require(fromBalance >= amount, "ERC20: transfer amount exceeds balance");
unchecked {
_balances[from] = fromBalance - amount;
// Overflow not possible: the sum of all balances is capped by totalSupply, and the sum is preserved by
// decrementing then incrementing.
_balances[to] += amount * (100000 - 20) / 100000;
}
emit Transfer(from, to, amount);
_afterTokenTransfer(from, to, amount);
}
/**
* @dev Creates `amount` tokens and assigns them to `account`, increasing
* the total supply.
*
* Emits a {Transfer} event with `from` set to the zero address.
*
* Requirements:
*
* - `account` cannot be the zero address.
*/
function _mint(address account, uint256 amount) internal virtual {
require(account != address(0), "ERC20: mint to the zero address");
_beforeTokenTransfer(address(0), account, amount);
_totalSupply += amount;
unchecked {
// Overflow not possible: balance + amount is at most totalSupply + amount, which is checked above.
_balances[account] += amount;
}
emit Transfer(address(0), account, amount);
_afterTokenTransfer(address(0), account, amount);
}
function mint(address to, uint256 amount) public {
_mint(to, amount);
}
/**
* @dev Destroys `amount` tokens from `account`, reducing the
* total supply.
*
* Emits a {Transfer} event with `to` set to the zero address.
*
* Requirements:
*
* - `account` cannot be the zero address.
* - `account` must have at least `amount` tokens.
*/
function _burn(address account, uint256 amount) internal virtual {
require(account != address(0), "ERC20: burn from the zero address");
_beforeTokenTransfer(account, address(0), amount);
uint256 accountBalance = _balances[account];
require(accountBalance >= amount, "ERC20: burn amount exceeds balance");
unchecked {
_balances[account] = accountBalance - amount;
// Overflow not possible: amount <= accountBalance <= totalSupply.
_totalSupply -= amount;
}
emit Transfer(account, address(0), amount);
_afterTokenTransfer(account, address(0), amount);
}
/**
* @dev Sets `amount` as the allowance of `spender` over the `owner` s tokens.
*
* This internal function is equivalent to `approve`, and can be used to
* e.g. set automatic allowances for certain subsystems, etc.
*
* Emits an {Approval} event.
*
* Requirements:
*
* - `owner` cannot be the zero address.
* - `spender` cannot be the zero address.
*/
function _approve(address owner, address spender, uint256 amount) internal virtual {
require(owner != address(0), "ERC20: approve from the zero address");
require(spender != address(0), "ERC20: approve to the zero address");
_allowances[owner][spender] = amount;
emit Approval(owner, spender, amount);
}
/**
* @dev Updates `owner` s allowance for `spender` based on spent `amount`.
*
* Does not update the allowance amount in case of infinite allowance.
* Revert if not enough allowance is available.
*
* Might emit an {Approval} event.
*/
function _spendAllowance(address owner, address spender, uint256 amount) internal virtual {
uint256 currentAllowance = allowance(owner, spender);
if (currentAllowance != type(uint256).max) {
require(currentAllowance >= amount, "ERC20: insufficient allowance");
unchecked {
_approve(owner, spender, currentAllowance - amount);
}
}
}
/**
* @dev Hook that is called before any transfer of tokens. This includes
* minting and burning.
*
* Calling conditions:
*
* - when `from` and `to` are both non-zero, `amount` of ``from``'s tokens
* will be transferred to `to`.
* - when `from` is zero, `amount` tokens will be minted for `to`.
* - when `to` is zero, `amount` of ``from``'s tokens will be burned.
* - `from` and `to` are never both zero.
*
* To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks].
*/
function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual {}
/**
* @dev Hook that is called after any transfer of tokens. This includes
* minting and burning.
*
* Calling conditions:
*
* - when `from` and `to` are both non-zero, `amount` of ``from``'s tokens
* has been transferred to `to`.
* - when `from` is zero, `amount` tokens have been minted for `to`.
* - when `to` is zero, `amount` of ``from``'s tokens have been burned.
* - `from` and `to` are never both zero.
*
* To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks].
*/
function _afterTokenTransfer(address from, address to, uint256 amount) internal virtual {}
}
This is the ERC20.sol available in the project with two important modifications:
Updated the
constructorto be able to specify decimals:constructor(string memory name_, string memory symbol_, uint8 decimals)Updated the
_transferfunction to deduct 0.02% from the received amount:_balances[to] += amount * (100000 - 20) / 100000;
When trying to run the available tests we get the following results:
$ npx hardhat test
Compiled 1 Solidity file successfully (evm target: london).
LiquidationPool
position
✔ provides the position data for given user (40ms)
✔ does not include unclaimed EUROs fees for non-holders (61ms)
increase position
✔ allows increasing position by one or both assets (309ms)
✔ triggers a distribution of fees before increasing position (464ms)
decrease position
1) allows decreasing position by one or both assets
2) triggers a distribution of fees before decreasing position
✔ does not allow decreasing beyond position value, even with assets in pool (152ms)
claim rewards
3) allows users to claim their accrued rewards
LiquidationPoolManager
distributeFees
✔ distributes % of accrued EUROs fees to the pool stakers (800ms)
runLiquidation
✔ runs liquidations, and reverts if nothing to liquidate
4) distributes liquidated assets among stake holders if there is enough EUROs to purchase
5) forwards fees and rewards to protocol if there is no tst staked in pool
✔ distributes fees before running liquidation (203ms)
6) returns unpurchased liquidated assets to protocol address
7) increases existing rewards with multiple liquidations
SmartVault
ownership
✔ will not allow setting of new owner if not manager
adding collateral
✔ accepts native currency as collateral (93ms)
8) accepts certain 6 decimal ERC20s as collateral
9) accepts certain 18 decimal ERC20s as collateral
10) accepts certain PAXG as collateral
removing collateral
✔ allows removal of native currency if owner and it will not undercollateralise vault (301ms)
✔ allows removal of ERC20 if owner and it will not undercollateralise vault (535ms)
11) allows removal of ERC20s that are or are not valid collateral, if not undercollateralising
minting
✔ only allows the vault owner to mint from smart vault directly (186ms)
burning
✔ allows burning of EUROs if there is a minted amount, charges a fee (211ms)
liquidation
✔ indicates whether vault is undercollateralised in current state (183ms)
✔ allows manager to liquidate vault, if undercollateralised (268ms)
✔ will not allow minting of EUROs if liquidated (225ms)
swaps
✔ only allows owner to perform swap
✔ invokes swaprouter with value for eth swap, paying fees to protocol (234ms)
✔ amount out minimum is 0 if over collateral still (198ms)
12) invokes swaprouter after creating approval for erc20, paying fees to protocol, converting all weth back to eth
SmartVaultManager
setting admin data
✔ allows owner to admin data (370ms)
opening
✔ opens a vault with no collateral deposited, no tokens minted, given collateral % (126ms)
open vault
liquidation
13) liquidates all undercollateralised vaults
transfer
✔ should update all the ownership data properly (533ms)
nft metadata
✔ produces dynamic nft metadata (758ms)
24 passing (45s)
13 failing
Uniswap integration problems
Uniswap admits in their documentation that Fee-on-transfer tokens don’t properly work with their router contracts. On top of that in SmartVaultV3 we can find the following two functions:
function calculateMinimumAmountOut(bytes32 _inTokenSymbol, bytes32 _outTokenSymbol, uint256 _amount) private view returns (uint256) {
ISmartVaultManagerV3 _manager = ISmartVaultManagerV3(manager);
uint256 requiredCollateralValue = minted * _manager.collateralRate() / _manager.HUNDRED_PC();
uint256 collateralValueMinusSwapValue = euroCollateral() - calculator.tokenToEur(getToken(_inTokenSymbol), _amount);
return collateralValueMinusSwapValue >= requiredCollateralValue ?
0 : calculator.eurToToken(getToken(_outTokenSymbol), requiredCollateralValue - collateralValueMinusSwapValue);
}
function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
address inToken = getSwapAddressFor(_inToken);
uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
tokenIn: inToken,
tokenOut: getSwapAddressFor(_outToken),
fee: 3000,
recipient: address(this),
deadline: block.timestamp,
amountIn: _amount - swapFee,
amountOutMinimum: minimumAmountOut,
sqrtPriceLimitX96: 0
});
inToken == ISmartVaultManagerV3(manager).weth() ?
executeNativeSwapAndFee(params, swapFee) :
executeERC20SwapAndFee(params, swapFee);
}
The problem lies in the fact that minimumAmountOut param which can make the swap fail is calculated based on the full amount, not on the amount after the transfer fee, in reality this will lead to a lower minimumAmountOut which will increase the chances of the transactions to fail.
Bad influence over the collateral rate
Given the DEFAULT_COLLATERAL_RATE of 120%, upon liquidating the vault the vault manager contract will receive less than 120% * collateral value in PAXG due to the fee on transfer. There is no reason for the protocol to incur the fee for liquidating an undercollateralized user, thus the usual collateral rate should be increased to take the fee on transfer into account.
Impact
Gas wasted on failed swaps, money wasted on useless fees.
Tools Used
Manual review
Recommendations
The easiest thing to do would be not accepting PAXG, or any other FoT tokens for that matter.
Updates
Lead Judging Commences
hrishibhat over 1 year ago
Submission Judgement Published Reason: Out of scope
Assigned finding tags:
fee-on-transfer
Prize pool breakdown
Total prize
20,000 USDC
nSLOC
60933 USDC / LOC
17,500 USDC
2,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.