Yeti Finance contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

Deleting a struct that contains a mapping will not delete the mapping contents:

  delete frontEndSnapshots[_frontEnd];

So when a mapping is re-initialized, it will have old values of this mapping.

Proof of Concept

Here is a simplified smart contract that demonstrates this issue in practice: https://gist.github.com/pauliax/67bc74982a78d28e94a175cf692830fb

1. invoke addTest with any address
2. see viewS
3. invoke deleteTest with the same address from step 1
4. see viewS

Handle

pauliax

Vulnerability details

Impact

contract ERC20_8 does not update _totalSupply on _mint/_burn, so it always remains 0.

Recommended Mitigation Steps

Increase total supply on mint and decrease on burn.

Handle

pauliax

Vulnerability details

Impact

  modifier exists(address _collateral) {
      if (validCollateral.length != 0 && validCollateral[0] != _collateral) {
          require(collateralParams[_collateral].index != 0, "collateral does not exists");
      }
      _;
  }

if validCollateral.length == 0, then the execution will continue as if the _collateral exists.

The sponsor confirmed that this behavior is not expected, although there should never be a situation where validCollateral.length is 0.

Handle

pauliax

Vulnerability details

Impact

There are functions that declare to return named variables but actually do not return anything. function _userUpdate declares to return pendingJoeSent, but actually does not return anything, so it always gets a default value of 0. function _sendCollateral should return a boolean value but does not return anything, so it also gets assigned a default value of false.

Handle

pauliax

Vulnerability details

Impact

function setAddresses in contract Whitelist is intended to be invoked only once (confirmed with the sponsor) but currently, it has no prevention from being called multiple times.

Maybe this should also be prevented in sYETIToken's setAddresses and ThreePieceWiseLinearPriceCurve's setAddresses.

Recommended Mitigation Steps

Prevent repeated access of setAddresses in Whitelist and potentially in sYETIToken and ThreePieceWiseLinearPriceCurve.

Handle

pauliax

Vulnerability details

Impact

There are many error messages that are quite long, e.g.:

  "YUSD: Cannot transfer tokens directly to the StabilityPool, TroveManager or BorrowerOps"

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met.

Handle

pauliax

Vulnerability details

Impact

Assigned operations to constant variables are re-evaluated every time:

      bytes32 private constant _PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");
    bytes32 private constant _TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
    
    uint constant public BORROWING_FEE_FLOOR = DECIMAL_PRECISION / 1000 * 5; // 0.5%
    uint constant public REDEMPTION_FEE_FLOOR = DECIMAL_PRECISION / 1000 * 5; // 0.5%
    
    uint constant public MAX_BORROWING_FEE = DECIMAL_PRECISION / 100 * 5; // 5%
    
    bytes32 private constant DOMAIN_SEPARATOR_SIGNATURE_HASH = keccak256("EIP712Domain(uint256 chainId,address verifyingContract)");

See https://github.com/ethereum/solidity/issues/9232

Recommended Mitigation Steps

Change from 'constant' to 'immutable'.

Handle

pauliax

Vulnerability details

Impact

Some contracts, e.g. ERC20_8, WJLP. are compiled with Solidity version >0.8, but still use SafeMath library. It is not necessary, as with this new version safe operations are handled by default.

Handle

pauliax

Vulnerability details

Impact

In function _transfer, shares.to128(); can be cached to skip the same calculation again:

  users[from].balance = fromUser.balance - shares.to128();
  users[to].balance = toUser.balance + shares.to128();

Same here, the result can be extracted to a constant as it never changes:

  (DECIMAL_PRECISION / 2)

Handle

pauliax

Vulnerability details

Impact

_isBeforeFeeBootstrapPeriod() is re-evaluated again and again inside the loop, although its value could be cached outside the loop and re-used to reduce gas costs.

Handle

pauliax

Vulnerability details

Impact

condition should be inclusive >= :

  if (available > totalClaimed.add(_amount))