WildcatMarketToken can revert on transferring full balance

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketToken.sol#L18

Vulnerability details

Impact

Under the hood WildcatMarketToken uses "scaled token amounts", while outside it operates with "normalized token amounts". Thus balanceOf() converts internal scaledAmount to normalized. Transfer on the contrary converts normalized amount to scaled.

Problem is that both these functions round up while converting.

Issue arises when in function balanceOf() value rounds up, such that scaledAmount < balanceOf() * scaleFactor. In this case transfer reverts if you try to send full balance. I suppose to look by example: 1) Suppose user's scaled (internal) balance is 5, scaleFactor = 0.5e27 2) He calls balanceOf() and gets 5 * 0.5 = 3 because function rounds up. 3) He calls transfer(3), function converts it to scaled balance 3 / 0.5 = 6. But his internal balance is 5 - therefore revert.

This inconsistency introduces serious problem for third party protocols that use WildcatMarketToken. Because to transfer full balance of contract is pretty normal behaviour, and in this case it reverts.

Proof of Concept

balanceOf uses rayMul() that rounds up:

  function balanceOf(address account) public view virtual nonReentrantView returns (uint256) {
    (MarketState memory state, , ) = _calculateCurrentState();
    return state.normalizeAmount(_accounts[account].scaledBalance);
  }

  function normalizeAmount(
    MarketState memory state,
    uint256 amount
  ) internal pure returns (uint256) {
    return amount.rayMul(state.scaleFactor);
  }

  /**
   * @dev Multiplies two ray, rounding half up to the nearest ray
   *      see https://twitter.com/transmissions11/status/1451131036377571328
   */
  function rayMul(uint256 a, uint256 b) internal pure returns (uint256 c) {
    ...
  }

_transfer() uses rayDiv() which rounds up too:

  function _transfer(address from, address to, uint256 amount) internal virtual {
    MarketState memory state = _getUpdatedState();
    uint104 scaledAmount = state.scaleAmount(amount).toUint104();
    ...
  }

  function scaleAmount(MarketState memory state, uint256 amount) internal pure returns (uint256) {
    return amount.rayDiv(state.scaleFactor);
  }

  /**
   * @dev Divide two ray, rounding half up to the nearest ray
   *      see https://twitter.com/transmissions11/status/1451131036377571328
   */
  function rayDiv(uint256 a, uint256 b) internal pure returns (uint256 c) {
    ...
  }

Insert this file to /test:

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.20;

import { Test } from 'forge-std/Test.sol';
import "../src/libraries/MathUtils.sol";
import "forge-std/Test.sol";

contract TestRounding is Test {

  function setUp() public {}

  function testRounding() public {
    uint256 scaleFactor = 0.5e27;
    uint256 scaledBalanceOfUser = 5;

    uint256 normalizedBalanceOfUser = MathUtils.rayMul(scaledBalanceOfUser, scaleFactor);
    uint256 scaledValueInTransfer = MathUtils.rayDiv(normalizedBalanceOfUser, scaleFactor);

    console.log("Try to transfer scaledValue: ", scaledValueInTransfer);
    console.log("Actual scaledBalance: ", scaledBalanceOfUser);

    assertTrue(scaledBalanceOfUser < scaledValueInTransfer);
  }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Round down in balanceOf. Something like:

  function balanceOf(address account) public view virtual nonReentrantView returns (uint256) {
    (MarketState memory state, , ) = _calculateCurrentState();
-   return state.normalizeAmount(_accounts[account].scaledBalance);
+   (_accounts[account].scaledBalance * state.scaleFactor) / RAY
  }

Assessed type

ERC20

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[d1ll0n]

This is actually impossible because scaleFactor can never be below 1e27.

You can prove this with solc's SMT solver by running:

solc test/TestRoundingError.sol --model-checker-engine bmc --model-checker-show-proved-safe

which outputs:

Info: BMC: Assertion violation check is safe!
  --> test/TestRoundingError.sol:20:5:
   |
20 |     assert(scaledAmount2 == scaledAmount);

// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.8.0;

uint constant RAY = 1e27;
uint constant HALF_RAY = 0.5e27;

contract TestRoundingError {
  function check_RoundingError(uint scaledAmount, uint scaleFactor) external pure {
    require(scaleFactor >= RAY && scaleFactor <= type(uint112).max);
    // Inline rayMul and rayDiv functions to help SMT solver parse the code

    // normalizedAmount = rayMul(scaledAmount, scaleFactor)
    require(scaledAmount <= (type(uint256).max - HALF_RAY) / scaleFactor);
    uint normalizedAmount = (scaledAmount * scaleFactor + HALF_RAY) / RAY;

    // scaledAmount2 = rayDiv(normalizedAmount, scaleFactor)
    uint halfScaleFactor = scaleFactor / 2;
    require(normalizedAmount <= (type(uint256).max - halfScaleFactor) / RAY);
    uint scaledAmount2 = (normalizedAmount * RAY + halfScaleFactor) / scaleFactor;
    assert(scaledAmount2 == scaledAmount);
  }
}

[c4-sponsor]

d1ll0n (sponsor) disputed

[c4-judge]

MarioPoneder marked the issue as unsatisfactory: Insufficient proof

[MarioPoneder]

Agree with the sponsor.
However, I want to point out the opportunity to submit a runnable PoC during post-judging QA to undisputably prove the issue.