The Standard
The Standard
DeFiHardhat
20,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Low severity issues(All compiled as one report)

0xrizwan

[L‑01] Missing address(0) checks for immutable state variables passed in constructors

Summary

Missing address(0) checks for immutable state variables passed in constructors

Vulnerability Details

In most of the-standard contracts, immutable variables value as argument is passed in constructor. Immutable variables are used as gas saving measure and if it is once set it can not be changed. The value of immutable variables is permanent in contract. If address(0) is passed in such variables then the contract would need to be redployed in worst case. Its better to prevent such errors by require validations so that such issues can be prevented.

Instance 1 at L-35 in SmartVaultV3.sol which can be checked here

It should be changed to,

constructor(bytes32 _native, address _manager, address _owner, address _euros, address _priceCalculator) {
+ require(_native != bytes(0), "invalid value");
+ require(_manager != address(0), "invalid address");
+ require(_owner != address(0), "invalid address");
+ require(address(_euros) != address(0), "invalid address");
+ require(address(_priceCalculator) != address(0), "invalid address");
NATIVE = _native;
owner = _owner;
manager = _manager;
EUROs = IEUROs(_euros);
calculator = IPriceCalculator(_priceCalculator);
}

Instance 2 at L-35 in LiquidationPool.sol which can be checked here

It should be changed to,

constructor(address _TST, address _EUROs, address _eurUsd, address _tokenManager) {
+ require(_TST != address(0), "invalid address");
+ require(_EUROs != address(0), "invalid address");
+ require(_eurUsd != address(0), "invalid address");
+ require(_tokenManager != address(0), "invalid address");
TST = _TST;
EUROs = _EUROs;
eurUsd = _eurUsd;
tokenManager = _tokenManager;
manager = payable(msg.sender);
}

Impact

Since immutable state varibales once set in constructor can not be reset, if address(0) is passed as arguments for such variables then contract would be required to re-deployed.

Tools Used

Manual review

Recommendations

Changes already mentioned in Vulnerability Details section to mitigate such issues.

[L‑02] LiquidationPool.distributeAssets() will always fail if manager does not approve allowance to LiquidationPool contract

Summary

LiquidationPool.distributeAssets() will always fail if manager does not approve allowance to LiquidationPool contract

Vulnerability Details

In LiquidationPool.distributeAssets() is used to distribute the assets to different holders. distributeAssets() token transfer logic is given as below,

​
function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable {
​
. . . some code
​
@> IERC20(asset.token.addr).safeTransferFrom(manager, address(this), _portion); @audiit // requires allowance from manager
}
​
. . . some code
​
}

The tokens are transferred to LiquidationPool contract from manager address and then these tokens can be claimed by holders by calling claim() function. The issue here is, the LiquidationPool contract requires allowance from manager otherwise the asset distribution will always fail.

The function is called in LiquidationPoolManager.sol contract by LiquidationPoolManager.runLiquidation() for liquidation run to specific tokenId.

Another issue is that, LiquidationPool.distributeAssets() can be called by anyone, however this should not be desired behavior.

Impact

LiquidationPool.distributeAssets() will always fail if manager does not approve allowance to LiquidationPool contract and the function can be called by anyone so it should only be called by manager address only i.e LiquidationPoolManager.sol contract.

Tools Used

Manual review

Recommendations

It is recommended to peform below changes in code,

```solidity
​
- function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable {
+ function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external onlyManager payable {
. . . some code
​
- IERC20(asset.token.addr).safeTransferFrom(manager, address(this), _portion);
+ IERC20(asset.token.addr).safeTransferFrom(msg.sender, address(this), _portion);
​
}
​
. . . some code
​
}

[L‑03] approve can fail for non standard ERC20 tokens like USDT

Summary

approve can fail for non standard ERC20 tokens like USDT

Vulnerability Details

Per the discussion with sponsors(@ewan),

In accepted tokens, will USDT and USDC be added in future as these are the most used tokens in crypto??

The response received:

no plans to yet, but it is possible yep

Therefore, all possible issues pertaining to USDC/USDT should be resolved in current code base as these tokens can be a part of accepted token by tokenManager. Therefore, below issue is specifically highlighting the issue with USDT approval failure in current code base.

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. For example Tether (USDT)’s approve() function will revert if the current approval is not zero, to protect against front-running changes of approvals. Link to usdt contract reference(SLOC 199-209)

approve is actually vulnerable to a sandwich attack as explained in the following document and this check for allowance doesn't actually avoid it.

Reference document link- https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit

In ERC20, front running attack is possible via approve() function,

Reference link for better understanding- https://blog.smartdec.net/erc20-approve-issue-in-simple-words-a41aaf47bca6

In the protocol, all functions using approve() must be first approved by zero. In LiquidationPoolManager.sol, runLiquidation() approves the ERC20 tokens i.e accepted tokens by tokenManager to pool contract.

function runLiquidation(uint256 _tokenId) external {
​
. . . some code
​
​
if (erc20balance > 0) {
assets[i] = ILiquidationPoolManager.Asset(token, erc20balance);
@> ierc20.approve(pool, erc20balance); @audit, approve fails in case of USDT
​
. . . some code
​
}

Impact

approve will fail in case of USDT due to not approving 0 first.

Tools Used

Manual review

Recommendations

Use OpenZeppelin’s SafeERC20 i.e forceApprove() or depending on openzeppelin version used etc, Alternatively, approve 0 first in case of USDT.

[L‑04] swap() should be payable to handle the ether transfer

Summary

swap() should be payable to handle the ether transfer

Vulnerability Details

In SmartVaultV3.swap() is used to swap between two tokens. The swap fee is handled in eth or erc20 tokens.

​
function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
address inToken = getSwapAddressFor(_inToken);
​
​
. . . some code
​
​
inToken == ISmartVaultManagerV3(manager).weth() ?
executeNativeSwapAndFee(params, swapFee) :
executeERC20SwapAndFee(params, swapFee);
}

Now, look into executeNativeSwapAndFee(params, swapFee)

​
function executeNativeSwapAndFee(ISwapRouter.ExactInputSingleParams memory _params, uint256 _swapFee) private {
(bool sent,) = payable(ISmartVaultManagerV3(manager).protocol()).call{value: _swapFee}(""); @audit // swap fee is handled in eth
require(sent, "err-swap-fee-native");
ISwapRouter(ISmartVaultManagerV3(manager).swapRouter2()).exactInputSingle{value: _params.amountIn}(_params);
}

The swap function would revert due to failure of swap fee in eth as the low level call function is expecting the value param in eth. Therefore the function should be payable to handle the swap fee in eth.

Impact

ether transfer would fail or revert if swap() does not have payable modifier

Tools Used

Manual review

Recommendations

Add payable keyword on swap()

[L‑05] claimRewards() would fail if the msg.sender is blacklisted in case of USDC/USDT

Summary

claim() would fail if the msg.sender is blacklisted in case of USDC/USDT

Vulnerability Details

Per the discussion with sponsors(@ewan),

In accepted tokens, will USDT and USDC be added in future as these are the most used tokens in crypto??

The response received:

no plans to yet, but it is possible yep

Therefore, issues pertaining to USDC/USDT should be accepted here. Below issue is highlighted if the claimer address is blacklisted by USDC/USDT.

USDC tokens have a blacklist() function which is used to blacklist any address by USDC admin. This can be checked here

Similarly, USDT tokens have a addToBlockedList() function which is used to blacklist any address by USDT admin. This can be checked here

It must be noted that USDC and USDT are widely used tokens in the crypto market and the potential accepted tokens as confirmed by sponsor discord message.

claim() function transfers the

function claimRewards() external {
ITokenManager.Token[] memory _tokens = ITokenManager(tokenManager).getAcceptedTokens();
for (uint256 i = 0; i < _tokens.length; i++) {
ITokenManager.Token memory _token = _tokens[i];
uint256 _rewardAmount = rewards[abi.encodePacked(msg.sender, _token.symbol)];
if (_rewardAmount > 0) {
delete rewards[abi.encodePacked(msg.sender, _token.symbol)];
if (_token.addr == address(0)) {
(bool _sent,) = payable(msg.sender).call{value: _rewardAmount}("");
require(_sent);
} else {
@> IERC20(_token.addr).transfer(msg.sender, _rewardAmount); @audit // claim revert in case msg.sender is blacklisted address
}
}
​
}
}

Consider a scenario,

  1. User wants to claim rewards and he calls LiquidationPool.claimRewards()

  2. The amount of rewards pertaining to user is fetched and it is transfered to msg.sender by transfer() function.

  3. The isssue arises if the user address i,e msg.sender is found to be blacklisted by USDC/USDT then the claimRewards() will always revert.

IERC20(_token.addr).transfer(msg.sender, _rewardAmount);

USDC/USDT contracts always check whether the calling address i.e msg.sender is blacklisted or not.

If this scenario happens which is very much likely the user won't be able to convert his rewards. It will break the one of the important functionality of contract.

Impact

claimRewards will fail and the rewards of users will be permanantly locked in contract if the claimer address is blacklisted

Tools Used

Manual review

Recommendations

Recommend to allow the recipient address as function argument and allow user to transfer the rewards tokens to his desired address.

For example:

- function claimRewards() external {
+ function claimRewards(address recipient) external {
+ require(recipient != address(0), "invalid address");
+ require(recipient != address(this), "invalid address");
ITokenManager.Token[] memory _tokens = ITokenManager(tokenManager).getAcceptedTokens();
for (uint256 i = 0; i < _tokens.length; i++) {
ITokenManager.Token memory _token = _tokens[i];
uint256 _rewardAmount = rewards[abi.encodePacked(msg.sender, _token.symbol)];
if (_rewardAmount > 0) {
delete rewards[abi.encodePacked(msg.sender, _token.symbol)];
if (_token.addr == address(0)) {
(bool _sent,) = payable(msg.sender).call{value: _rewardAmount}("");
require(_sent);
} else {
- IERC20(_token.addr).transfer(msg.sender, _rewardAmount);
+ IERC20(_token.addr).transfer(recipient, _rewardAmount);
}
}
}
}
Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

informational/invalid

0xrizwan Submitter
over 1 year ago
hrishibhat Lead Judge
over 1 year ago
hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.