search

code-423n4 / 2022-07-swivel-findings

0 stars 1 forks source link

ZcToken: previewWithdraw should be rounded up #8

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Creator/ZcToken.sol#L88-L92

Vulnerability details

Impact

According to https://eips.ethereum.org/EIPS/eip-5095, the result of previewWithdraw should be rounded up.

    function previewWithdraw(uint256 underlyingAmount) external override view returns (uint256 principalAmount){
        if (block.timestamp < maturity) {
            return 0;
        }
        return (underlyingAmount * IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate / IRedeemer(redeemer).getExchangeRate(protocol, cToken));
    }

This makes the ZcToken non-compliant with the EIP-5095 standard and may cause some integration problems.

Proof of Concept

https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Creator/ZcToken.sol#L88-L92 https://eips.ethereum.org/EIPS/eip-5095

Tools Used

None

Recommended Mitigation Steps

round up the result of previewWithdraw

JTraversa commented 2 years ago

As mentioned in scope, the specific implementation details of 5095 are not included given the EIP isnt final (and we're one of the contributors working on it)

bghughes commented 2 years ago

As mentioned in scope, the specific implementation details of 5095 are not included given the EIP isnt final (and we're one of the contributors working on it)

Agreed, per the terms of the contest:

Areas To Ignore:

While already noted, there are a couple areas to ignore:

  1. Non-Impactful or automatically reverting input sanitization.
  2. Non-Impactful and/or already delayed admin functionality.
  3. Non-Compliance with 100% of EIP-5095 (it is still a draft)

Downgrading to QA/Informational

bghughes commented 2 years ago

Grouping this with the warden’s QA report, #34