The implementation of the max withdrawable amount is incorrect as it divides the calculation by the wrong denominator, leading to an incorrect result and a potential denial of service due to an overflow.
In the Application Specific Dollar protocol, users mint asD tokens by depositing NOTE tokens which are forwarded to the Canto Lending Market and exchanged for cNOTE tokens. For each deposited NOTE, users are minted an equal amount of asD tokens. Users can later withdraw their NOTE by burning their asD token, which removes the underlying tokens from the Canto Lending Market by exchanging the cNOTE for NOTE and returns them to the user. Interests generated in the Canto Lending Market can be withdrawn by the owner of this specific asD token.
Owners are limited to withdraw just the interests generated from the deposited NOTE. Users of the protocol should be able to redeem any amount of asD tokens at any time, which means that there must always be enough cNOTE to cover for the total supply of asD tokens. The owner should not be able to withdraw past this limit, which is implemented in the function withdrawCarry():
72: function withdrawCarry(uint256 _amount) external onlyOwner {
73: uint256 exchangeRate = CTokenInterface(cNote).exchangeRateCurrent(); // Scaled by 1 * 10^(18 - 8 + Underlying Token Decimals), i.e. 10^(28) in our case
74: // The amount of cNOTE the contract has to hold (based on the current exchange rate which is always increasing) such that it is always possible to receive 1 NOTE when burning 1 asD
75: uint256 maximumWithdrawable = (CTokenInterface(cNote).balanceOf(address(this)) * exchangeRate) /
76: 1e28 -
77: totalSupply();
78: if (_amount == 0) {
79: _amount = maximumWithdrawable;
80: } else {
81: require(_amount <= maximumWithdrawable, "Too many tokens requested");
82: }
83: // Technically, _amount can still be 0 at this point, which would make the following two calls unnecessary.
84: // But we do not handle this case specifically, as the only consequence is that the owner wastes a bit of gas when there is nothing to withdraw
85: uint256 returnCode = CErc20Interface(cNote).redeemUnderlying(_amount);
86: require(returnCode == 0, "Error when redeeming"); // 0 on success: https://docs.compound.finance/v2/ctokens/#redeem
87: IERC20 note = IERC20(CErc20Interface(cNote).underlying());
88: SafeERC20.safeTransfer(note, msg.sender, _amount);
89: emit CarryWithdrawal(_amount);
90: }Lines 75-77 calculate the maximum withdrawable amount the owner can remove. This is done by multiplying the current balance of cNOTE for the exchange rate, dividing by the normalization factor, and then subtracting the supply of asD. The intention is quite simple: we take the cNOTE balance, translate it to NOTE using the exchange rate, then subtract the total supply of asD (to account for every deposit made in the protocol), and that yields the amount entitled to the owner.
However, the applied normalization factor after the balance is multiplied by the exchange rate is incorrect. The value is not 1e28, it should be 1e18. This can be seen in the logic of Compound: let's drill down on the redeem() function which is used to exchange cTokens for the underlying token, which will let us see how to correctly convert cTokens into the underlying token. redeem() calls redeemInternal(), which ends up calling redeemFresh(payable(msg.sender), redeemTokens, 0):
480: function redeemFresh(address payable redeemer, uint redeemTokensIn, uint redeemAmountIn) internal {
481: require(redeemTokensIn == 0 || redeemAmountIn == 0, "one of redeemTokensIn or redeemAmountIn must be zero");
482:
483: /* exchangeRate = invoke Exchange Rate Stored() */
484: Exp memory exchangeRate = Exp({mantissa: exchangeRateStoredInternal() });
485:
486: uint redeemTokens;
487: uint redeemAmount;
488: /* If redeemTokensIn > 0: */
489: if (redeemTokensIn > 0) {
490: /*
491: * We calculate the exchange rate and the amount of underlying to be redeemed:
492: * redeemTokens = redeemTokensIn
493: * redeemAmount = redeemTokensIn x exchangeRateCurrent
494: */
495: redeemTokens = redeemTokensIn;
496: redeemAmount = mul_ScalarTruncate(exchangeRate, redeemTokensIn);
497: } else {When redeemTokensIn > 0 (our case) the implementation calls mul_ScalarTruncate() using the exchange rate and the amount of cTokens in order to obtain the amount of underlying tokens. mul_ScalarTruncate() will multiply both numbers and then call truncate() on the product, which divides by the expScale factor.
37: function mul_ScalarTruncate(Exp memory a, uint scalar) pure internal returns (uint) {
38: Exp memory product = mul_(a, scalar);
39: return truncate(product);
40: }116: function mul_(Exp memory a, uint b) pure internal returns (Exp memory) {
117: return Exp({mantissa: mul_(a.mantissa, b)});
118: }
136: function mul_(uint a, uint b) pure internal returns (uint) {
137: return a * b;
138: }29: function truncate(Exp memory exp) pure internal returns (uint) {
30: // Note: We are not using careful math here as we're performing a division that cannot fail
31: return exp.mantissa / expScale;
32: }Since expScale is 1e18, the operation to calculate the amount of underlying tokens given an amount of cTokens is effectively cTokens * exchangeRate / 1e18.
This means that the normalization factor in the calculation of maximumWithdrawable is off by 1e10, causing the calculation not only to be incorrect, but to likely cause a denial of service due to a math overflow when subtracting the total supply. Since the number is being divided by a bigger amount than should be, the implementation will end up subtracting a larger number, causing an overflow due to the result being negative.
To simplify the demonstration and better show the issue, we will mimic the operations that are done internally in withdrawCarry() of the asD contract.
In the scenario, we have 100e18 total supply of asD tokens, and we also assume the contract holds the same quantity of cNOTE tokens. We can think of this as if the exchange rate was 1=1 when the NOTE was deposited in the Canto Lending Market. The test forks the Canto network to test against the real current exchange rate of the deployed Canto Lending Market.
When the test executes the calculation in the commented line (cNoteBalance * exchangeRate) / 1e28 - totalSupply, it will revert due to an arithmetic overflow.
When dividing by 1e18, the calculation returns the correct amount, that aligns perfectly when redeeming all the cNOTE tokens and comparing it to the total supply of asD, which represents the NOTE balance owned by depositors of the protocol (maximumWithdrawable == noteBalance - totalSupply).
The test should be executed by forking the Canto network:
forge test -vvv --mc AuditTest --fork-url=wss://canto.gravitychain.io:8546Note: the snippet shows only the relevant code for the test. Full test file can be found here.
function test_withdrawCarry_IncorrectMaxWithdrawable() public {
CTokenInterface cNote = CTokenInterface(CNOTE);
IERC20 note = IERC20(NOTE);
// Simulate we are operating under the asD contract
address asD = makeAddr("asD");
// Let's assume the total supply in asD is 100e18 (meaning we minted on 100e18 $NOTE)
uint256 totalSupply = 100e18;
// And let's assume we have 100e18 cNote tokens
deal(CNOTE, asD, 100e18);
// Log cNOTE balance (should be 100e18)
uint256 cNoteBalance = cNote.balanceOf(asD);
console.log("Initial cNote balance:", cNoteBalance);
// Calc maxWithdraw
uint256 exchangeRate = CTokenInterface(cNote).exchangeRateCurrent();
// The following will overflow! due to the division of 1e28 instead of 1e18
// uint256 maximumWithdrawable = (cNoteBalance * exchangeRate) / 1e28 - totalSupply;
uint256 maximumWithdrawable = (cNoteBalance * exchangeRate) / 1e18 - totalSupply;
console.log("Correct maximumWithdrawable:", maximumWithdrawable);
// asD redeems all cNote
vm.prank(asD);
CErc20Interface(CNOTE).redeem(cNoteBalance);
// Log NOTE balance
uint256 noteBalance = note.balanceOf(asD);
console.log("Note balance after redeem:", noteBalance);
// Check the maximumWithdrawable is ok
assertEq(maximumWithdrawable, noteBalance - totalSupply);
}The calculation of maximumWithdrawable in withdrawCarry() should divide by 1e18 instead of 1e28.
uint256 maximumWithdrawable = (CTokenInterface(cNote).balanceOf(address(this)) * exchangeRate) /
- 1e28 -
+ 1e18 -
totalSupply();