Ondo Finance contest - adriro's results

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L707-L727

Vulnerability details

The function completeRedemptions present in the CashManager contract is used by the manager to complete redemptions requested by users and also to process refunds.

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L707-L727

function completeRedemptions(
  address[] calldata redeemers,
  address[] calldata refundees,
  uint256 collateralAmountToDist,
  uint256 epochToService,
  uint256 fees
) external override updateEpoch onlyRole(MANAGER_ADMIN) {
  _checkAddressesKYC(redeemers);
  _checkAddressesKYC(refundees);
  if (epochToService >= currentEpoch) {
    revert MustServicePastEpoch();
  }
  // Calculate the total quantity of shares tokens burned w/n an epoch
  uint256 refundedAmt = _processRefund(refundees, epochToService);
  uint256 quantityBurned = redemptionInfoPerEpoch[epochToService]
    .totalBurned - refundedAmt;
  uint256 amountToDist = collateralAmountToDist - fees;
  _processRedemption(redeemers, amountToDist, quantityBurned, epochToService);
  collateral.safeTransferFrom(assetSender, feeRecipient, fees);
  emit RedemptionFeesCollected(feeRecipient, fees, epochToService);
}

The total refunded amount that is returned from the internal call to _processRefund is then used to calculate the effective amount of CASH burned (redemptionInfoPerEpoch[epochToService].totalBurned - refundedAmt). This resulting value is then used to calculate how much each user should receive based on how much CASH they redeemed and the total amount that was burned.

The main issue here is that the refunded amount is not updated in the totalBurned storage variable for the given epoch. Any subsequent call to this function won't take into account refunds from previous calls.

Impact

If the manager completes the refunds and redemptions at different steps or stages for a given epoch, using multiple calls to the completeRedemptions, then any refunded amount won't be considered in subsequent calls to the function.

Any redemption that is serviced in a call after a refund will be calculated using the total burned without subtracting the previous refunds. The function completeRedemptions will call the internal function _processRedemption passing the burned amount as the quantityBurned argument, the value is calculated in line 755:

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L755

uint256 collateralAmountDue = (amountToDist * cashAmountReturned) /
        quantityBurned;

This means that redemptions that are processed after one or more previous refunds will receive less collateral tokens even if they redeemed the same amount of CASH tokens (i.e. greater quantityBurned, less collateralAmountDue), causing loss of funds for the users.

PoC

In the following test, Alice, Bob and Charlie request a redemption. The admin first calls completeRedemptions to process Alice's request and refund Charlie. The admin then makes a second call to completeRedemptions to process Bob's request. Even though they redeemed the same amount of CASH (each 200e18), Alice gets 150e6 tokens while Bob is sent ~133e6.

contract TestAudit is BasicDeployment {
    function setUp() public {
        createDeploymentCash();

        // Grant Setter
        vm.startPrank(managerAdmin);
        cashManager.grantRole(cashManager.SETTER_ADMIN(), address(this));
        cashManager.grantRole(cashManager.SETTER_ADMIN(), managerAdmin);
        vm.stopPrank();

        // Seed address with 1000000 USDC
        vm.prank(USDC_WHALE);
        USDC.transfer(address(this), INIT_BALANCE_USDC);
    }

    function test_CashManager_completeRedemptions_BadReedem() public {
        _setupKYCStatus();

        // Seed alice and bob with 200 cash tokens
        _seed(200e18, 200e18, 50e18);

        // Have alice request to withdraw 200 cash tokens
        vm.startPrank(alice);
        tokenProxied.approve(address(cashManager), 200e18);
        cashManager.requestRedemption(200e18);
        vm.stopPrank();

        // Have bob request to withdraw 200 cash tokens
        vm.startPrank(bob);
        tokenProxied.approve(address(cashManager), 200e18);
        cashManager.requestRedemption(200e18);
        vm.stopPrank();

        // Have charlie request to withdraw his tokens
        vm.startPrank(charlie);
        tokenProxied.approve(address(cashManager), 50e18);
        cashManager.requestRedemption(50e18);
        vm.stopPrank();

        // Move forward to the next epoch
        vm.warp(block.timestamp + 1 days);
        vm.prank(managerAdmin);
        cashManager.setMintExchangeRate(2e6, 0);

        // Approve the cashMinter contract from the assetSender account
        _seedSenderWithCollateral(300e6);

        // First call, withdraw Alice and refund Charlie
        address[] memory withdrawFirstCall = new address[](1);
        withdrawFirstCall[0] = alice;
        address[] memory refundFirstCall = new address[](1);
        refundFirstCall[0] = charlie;

        vm.prank(managerAdmin);
        cashManager.completeRedemptions(
            withdrawFirstCall, // Addresses to issue collateral to
            refundFirstCall, // Addresses to refund cash
            300e6, // Total amount of money to dist incl fees
            0, // Epoch we wish to process
            0 // Fee amount to be transferred to ondo
        );

        // Alice redemption is calculated taking the refund into account
        uint256 aliceExpectedBalance = 200e18 * 300e6 / ((200e18 + 200e18 + 50e18) - 50e18);
        assertEq(USDC.balanceOf(alice), aliceExpectedBalance);
        assertEq(USDC.balanceOf(bob), 0);
        assertEq(tokenProxied.balanceOf(charlie), 50e18);

        // Second call, withdraw Bob
        address[] memory withdrawSecondCall = new address[](1);
        withdrawSecondCall[0] = bob;
        address[] memory refundSecondCall = new address[](0);

        vm.prank(managerAdmin);
        cashManager.completeRedemptions(
            withdrawSecondCall, // Addresses to issue collateral to
            refundSecondCall, // Addresses to refund cash
            300e6, // Total amount of money to dist incl fees
            0, // Epoch we wish to process
            0 // Fee amount to be transferred to ondo
        );

        // But here, Bob's redemption doesn't consider the previous refund.
        uint256 bobBadBalance = uint256(200e18 * 300e6) / (200e18 + 200e18 + 50e18);
        assertEq(USDC.balanceOf(bob), bobBadBalance);
    }

    function _setupKYCStatus() internal {
        // Add KYC addresses
        address[] memory addressesToKYC = new address[](5);
        addressesToKYC[0] = guardian;
        addressesToKYC[1] = address(cashManager);
        addressesToKYC[2] = alice;
        addressesToKYC[3] = bob;
        addressesToKYC[4] = charlie;
        registry.addKYCAddresses(kycRequirementGroup, addressesToKYC);
    }

    function _seed(
        uint256 aliceAmt,
        uint256 bobAmt,
        uint256 charlieAmt
    ) internal {
        vm.startPrank(guardian);
        tokenProxied.mint(alice, aliceAmt);
        tokenProxied.mint(bob, bobAmt);
        tokenProxied.mint(charlie, charlieAmt);
        vm.stopPrank();
    }

    function _seedSenderWithCollateral(uint256 usdcAmount) internal {
        vm.prank(USDC_WHALE);
        USDC.transfer(assetSender, usdcAmount);
        vm.prank(assetSender);
        USDC.approve(address(cashManager), usdcAmount);
    }
}

Recommendation

Update the totalBurned amount to consider refunds resulting from the call to _processRefund:

  function completeRedemptions(
    address[] calldata redeemers,
    address[] calldata refundees,
    uint256 collateralAmountToDist,
    uint256 epochToService,
    uint256 fees
  ) external override updateEpoch onlyRole(MANAGER_ADMIN) {
    _checkAddressesKYC(redeemers);
    _checkAddressesKYC(refundees);
    if (epochToService >= currentEpoch) {
      revert MustServicePastEpoch();
    }
    // Calculate the total quantity of shares tokens burned w/n an epoch
    uint256 refundedAmt = _processRefund(refundees, epochToService);
    uint256 quantityBurned = redemptionInfoPerEpoch[epochToService]
      .totalBurned - refundedAmt;
+   redemptionInfoPerEpoch[epochToService].totalBurned = quantityBurned;
    uint256 amountToDist = collateralAmountToDist - fees;
    _processRedemption(redeemers, amountToDist, quantityBurned, epochToService);
    collateral.safeTransferFrom(assetSender, feeRecipient, fees);
    emit RedemptionFeesCollected(feeRecipient, fees, epochToService);
  }

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/kyc/KYCRegistry.sol#L79-L112

Vulnerability details

The function addKYCAddressViaSignature of the KYCRegistry contract allows a user to be granted a KYC status using a signature provided by Ondo. The function validates that the signer has the corresponding role for the requirement group and adds the user to the given KYC group if successful.

While the signed digest contains a deadline, if this deadline is long enough, a user may be able to reuse the provided signature.

Impact

The signature can be reused to add the user back to the registry within the limits of the deadline.

If the user is first KYC approved off-chain and given a signature, and is later removed from the registry by a privileged role using removeKYCAddresses, then the user can resubmit the signature to addKYCAddressViaSignature and be readded to the KYC registry.

PoC

The following test illustrates the issue.

contract TestAudit is BasicDeployment {
    function setUp() public {
        createDeploymentCash();

        // Grant Setter
        vm.startPrank(managerAdmin);
        cashManager.grantRole(cashManager.SETTER_ADMIN(), address(this));
        cashManager.grantRole(cashManager.SETTER_ADMIN(), managerAdmin);
        vm.stopPrank();

        // Seed address with 1000000 USDC
        vm.prank(USDC_WHALE);
        USDC.transfer(address(this), INIT_BALANCE_USDC);
    }
    
    function test_KYCRegistry_addKYCAddressViaSignature_ReuseSignature() public {
        uint256 signerPrivateKey = 0x123;
        address signer = vm.addr(signerPrivateKey);

        // Add signer KEY group role
        vm.prank(registryAdmin);
        registry.grantRole(KYC_GROUP_2_ROLE, signer);

        // Alice is not in the registry
        assertFalse(registry.getKYCStatus(kycRequirementGroup, alice));

        // // Alice is KYC off-chain and provided a signature
        uint256 deadline = block.timestamp + 30 days;
        bytes32 structHash = keccak256(abi.encode(registry._APPROVAL_TYPEHASH(), kycRequirementGroup, alice, deadline));
        bytes32 hash = ECDSA.toTypedDataHash(registry.DOMAIN_SEPARATOR(), structHash);
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, hash);

        vm.prank(alice);
        registry.addKYCAddressViaSignature(kycRequirementGroup, alice, deadline, v, r, s);

        // Alice is now KYC approved
        assertTrue(registry.getKYCStatus(kycRequirementGroup, alice));

        // Simulate time passes...
        vm.warp(block.timestamp + 15 days);

        // Admin removes Alice KYC status
        address[] memory addressesToRemoveKYC = new address[](1);
        addressesToRemoveKYC[0] = alice;
        vm.prank(signer);
        registry.removeKYCAddresses(kycRequirementGroup, addressesToRemoveKYC);
        assertFalse(registry.getKYCStatus(kycRequirementGroup, alice));

        // Alice reuses the signature and gets back her KYC status
        vm.prank(alice);
        registry.addKYCAddressViaSignature(kycRequirementGroup, alice, deadline, v, r, s);
        assertTrue(registry.getKYCStatus(kycRequirementGroup, alice));
    }
}

Recommendation

Use a nonce for each account and include it as part of the signed message to prevent reusing the same signature more than once. When calling addKYCAddressViaSignature, validate that the nonce is the current nonce for the given user. If successful, consume and increment the nonce.

Unneeded initialization in CashManager constructor

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L168

The currentEpoch variable is initialized with the same variable.

setPendingMintBalance doesn't check for mint limit

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L336-L350

The function setPendingMintBalance of the CashManager contract allows the manager to set the mint balance for a user, but fails to check and update the mint limit (mintLimit variable).

overrideExchangeRate fails to validate the new exchange rate

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L366-L385

The overrideExchangeRate function of the CashManager contract should validate that the new rate is not 0 (correctExchangeRate != 0).

Validate new config parameter is not address(0)

Occurrences:

• https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L452
• https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L465

The sweepToken function is missing in the CErc20DelegatorKYC contract

The sweepToken function which is implemented in the original Compound contract (https://github.com/compound-finance/compound-protocol/blob/3affca87636eecd901eb43f81a4813186393905d/contracts/CErc20Delegator.sol#L330-L336) is missing in the CErc20DelegatorKYC contract.

Redundant validation in _checkAndUpdateRedeemLimit

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L642

The function _checkAndUpdateRedeemLimit in the CashManager contract validates that the amount is not 0, but this is redundant since the same amount has been validated against in the minimumRedeemAmount variable as part of the requestRedemption function.

Usage of safe unchecked math

• CashManager line 865 can be unchecked due to the if guard in line 864.
  https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L865

• CashManager line 867 can be unchecked due to the if guard in line 866.
  https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L867

Cash factories deploy a new implementation contract for each proxy

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashFactory.sol#L79 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashKYCSenderFactory.sol#L81 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashKYCSenderReceiverFactory.sol#L81

All three factories (CashFactory, CashKYCSenderFactory and deployCashKYCSenderReceiver) deploy an individual implementation contract when deploying a new proxy.

This is not needed and can be safely removed. New proxies can point to the same implementation to save gas.

Cash factories store a reference to the latest set of deployed contract in storage

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashFactory.sol#L49-L51 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashKYCSenderFactory.sol#L49-L51 https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashKYCSenderReceiverFactory.sol#L49-L51

Each of the three factories (CashFactory, CashKYCSenderFactory and deployCashKYCSenderReceiver) will store a reference to each deployed contract (implementation, proxy and admin) in storage when a new token is deployed.

This shouldn't be needed and probably doesn't make any sense, and can be removed to save gas.

Simplify KYC group roles in KYCRegistry

The KYCRegistry contract keeps a mapping between group level and the role identifier assigned to that group.

This logic may be simplified by taking the role identifier as a predefined function of the group level. For example, using bytes32 GROUP_ROLE = keccak(kycRequirementGroup).

This eliminates the need to maintain the kycGroupRoles, which can be removed, and saves one storage lookup when calling the function of the KYCRegistry contract.