Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

`CapitalPool:approve` Will Not Work With Certain Tokens That Do Not Support approve `type(uint256).max`

0xdarko

Summary

There are certain ERC20 tokens that do not support an approval amount type(uint256).max (e.g. USDT). If this is changed to accommodate these tokens, additional steps are needed for other tokens that require setting allowance to zero before changing it=.

Vulnerability Details

  1. The ERC20 tokens that don't accept type(uint256).max will cause a revert in CapitalPool:approve.

  2. If type(uint256).max is replaced, some tokens (e.g., USDT) require setting the allowance to zero before changing it to a non-zero value.

function approve(address tokenAddr) external {
// ... OTHER CODE ... //
​
(bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR,
tokenManager,
@> type(uint256).max
)
);
​
if (!success) {
revert ApproveFailed();
}
}

Impact

  1. Not being able to approve certain tokens with type(uint256).max leads to not being able to approve those tokens to be able to be withdrawn from TokenManager::withdraw.

  2. If type(uint256).max is replaced without considering tokens requiring zero-setting, it could lead to failed approvals for these tokens.

Tools Used

Foundry

Recommendations

  1. Import IERC20 Interface and change approve:

+ import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
​
function approve(address tokenAddr) external {
// ... OTHER CODE ... //
+ uint256 approveAmount = IERC20(tokenAddr).balanceOf(address(this));
​
(bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR,
tokenManager,
- type(uint256).max
+ approveAmount
)
);

  1. If incorporating resetting allowance to zero:

+ import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
​
+ error ApproveToZeroFailed();
​
function approve(address tokenAddr) external {
// ... OTHER CODE ... //
+ uint256 currentAllowance = IERC20(tokenAddr).allowance(address(this), tokenManager);
+ uint256 approveAmount = IERC20(tokenAddr).balanceOf(address(this));
​
+ // Set allowance to zero if it's not already zero
+ if (currentAllowance != 0) {
+ (bool successZero, ) = tokenAddr.call(
+ abi.encodeWithSelector(APPROVE_SELECTOR, tokenManager, 0)
+ );
+ if (!successZero) revert ApproveToZeroFailed();
+ }
​
(bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR,
tokenManager,
- type(uint256).max
+ approveAmount
)
);
Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

[invalid] finding-CapitalPool-approve-uint256-max

Thanks for flagging, indeed since uint(-1) is representative of max uint256 value, when entering the `if` statement, it will be converted to uint96 max amout, so it will not revert as described. In issue #361, the mockToken utilized does not correctly reflect the below approval behavior. ```Solidity function approve(address spender, uint rawAmount) external returns (bool) { uint96 amount; if (rawAmount == uint(-1)) { amount = uint96(-1); } else { amount = safe96(rawAmount, "Comp::approve: amount exceeds 96 bits"); } ```

Appeal created

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[invalid] finding-CapitalPool-approve-uint256-max

Thanks for flagging, indeed since uint(-1) is representative of max uint256 value, when entering the `if` statement, it will be converted to uint96 max amout, so it will not revert as described. In issue #361, the mockToken utilized does not correctly reflect the below approval behavior. ```Solidity function approve(address spender, uint rawAmount) external returns (bool) { uint96 amount; if (rawAmount == uint(-1)) { amount = uint96(-1); } else { amount = safe96(rawAmount, "Comp::approve: amount exceeds 96 bits"); } ```

Support

FAQs

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

Give us feedback!