Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Mismatch between the natspec comment and the documentation.

0xakira

Vulnerability details

Mismatch between the natspec comment and the documentation. In scope to the contest it is specified that collateral token will be only WETH, LINK USDC, WBTC but in PerpetualVault::initialize function in natspec comment it is specified that collateral token can be ETH, WETH, BTC, LINK, UNI, USDC, USDT, DAI, FRAX.

/**
* @notice
@> * `collateralToken` can be ETH, WETH, BTC, LINK, UNI, USDC, USDT, DAI, FRAX.
* @param _market address of GMX market
* @param _keeper keeper address
* @param _treasury fee receiver
* @param _gmxProxy address of GMXUtils contract
* @param _minDepositAmount minimum deposit amount
* @param _maxDepositAmount maximum deposit amount
*/
function initialize(

Because of this, it can be confusing for developers and users.

Impact

If you don't consider adding these tokens in the future, the vulnerability is that users can send ETH to the contract and thus lose it. If you are going to add these tokens in future implementations, users can also lose e.g. ETH, thinking that there is a conversion of ether to WETH and vice versa. There is also a vulnerability for using the USDT token. The _transferToken function is called in the _handleReturn function, which in turn passes the token to the recipient in the try block and in case of revert handles this case in the catch block where it sends this token to the treasury. The vulnerability is that USDT is a weird token and in case of an error it will return false, but the catch block does not catch errors as false. The token is transferred via normal transfer, not safeTransfer. So the user may not get his tokens as well as the treasury.

function _transferToken(uint256 depositId, uint256 amount) internal {
uint256 fee;
if (amount > depositInfo[depositId].amount) {
fee = (amount - depositInfo[depositId].amount) * governanceFee / BASIS_POINTS_DIVISOR;
if (fee > 0) {
collateralToken.safeTransfer(treasury, fee);
}
}
try collateralToken.transfer(depositInfo[depositId].recipient, amount - fee) {}
catch {
collateralToken.transfer(treasury, amount - fee);
emit TokenTranferFailed(depositInfo[depositId].recipient, amount - fee);
}
totalDepositAmount -= depositInfo[depositId].amount;
emit GovernanceFeeCollected(address(collateralToken), fee);
}

Recommended Mitigation Steps

Remove unnecessary tokens in natSpec comments, or optimise the functions for all these tokens

Updates

Lead Judging Commences

n0kto Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

n0kto Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Support

FAQs

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