ProtectedERC20Gateway Manual Review Findings

PEC-01M: Potential Withdrawal Denial-of-Service

Type	Severity	Location
Language Specific		ProtectedERC20Gateway.sol:L106

Description:

The ProtectedERC20Gateway::withdrawProtectedTokenWithPermit function will attempt to consume a permit before executing the ERC20::transferFrom operation. Given that permits can be submitted and consumed by anyone, it is possible for a user to detect a substantial ProtectedERC20Gateway::withdrawProtectedTokenWithPermit operation and sabotage it by consuming the permit first.

Impact:

While the Denial-of-Service attack is trivial to perform, its impact is minimal as it would only be important in case a substantial withdrawal is detected and wished to be arbitraged.

Example:

Copy
88function withdrawProtectedTokenWithPermit(
89  address pToken,
90  uint256 amount,
91  address to,
92  uint256 deadline,
93  uint8 permitV,
94  bytes32 permitR,
95  bytes32 permitS
96) external {
97  IAToken apToken = IAToken(POOL.getReserveData(pToken).aTokenAddress);
98  uint256 userBalance = apToken.balanceOf(msg.sender);
99  uint256 amountToWithdraw = amount;
100
101  // if amount is equal to type(uint256).max, the user wants to redeem everything
102  if (amount == type(uint256).max) {
103    amountToWithdraw = userBalance;
104  }
105  // permit `amount` rather than `amountToWithdraw` to make it easier for front-ends and integrators
106  apToken.permit(msg.sender, address(this), amount, deadline, permitV, permitR, permitS);
107  apToken.transferFrom(msg.sender, address(this), amountToWithdraw);
108
109  POOL.withdraw(pToken, amountToWithdraw, address(this));
110  IPERC20(pToken).withdrawTo(to, amountToWithdraw);
111}

Recommendation:

We advise the code to opportunistically perform the IAToken::permit operation, continuing with the ERC20::transferFrom operation regardless of whether the permit succeeds as it may have been consumed by another party.

Alleviation:

The Kinza Finance team has stated that while they acknowledge this exhibit they consider that it bears no financial impact and as such will acknowledge it.

PEC-02M: Potentially Incompatible Approval Methodologies

Type	Severity	Location
Standard Conformity		ProtectedERC20Gateway.sol:L43, L47

Description:

The referenced statements are meant to reset the approval of the contract to the target to the maximum value possible, however, this reset can occur when a non-zero approval already occurs.

Impact:

Some tokens will be incompatible with the approval reset mechanism of the ProtectedERC20Gateway::depositProtectedToken function.

Example:

Copy
34function depositProtectedToken(
35  IPERC20 pToken,
36  address onBehalfOf,
37  uint256 amount,
38  uint16 referralCode
39) external {
40  IERC20 token = IERC20(pToken.underlying());
41  token.transferFrom(msg.sender, address(this), amount);
42  if (token.allowance(address(this), address(pToken)) < amount) {
43      token.approve(address(pToken), type(uint256).max);
44  }
45  pToken.depositFor(address(this), amount);
46  if (pToken.allowance(address(this), address(POOL)) < amount) {
47      pToken.approve(address(POOL), type(uint256).max);
48  }
49  // if pToken does not exist as an reserve, this would revert
50  POOL.deposit(address(pToken), amount, onBehalfOf, referralCode);
51}

Recommendation:

As some tokens will not allow a non-zero approval to occur when a non-zero approval already exists, we advise the code to reset the previous approval and then assign it to the value of type(uint256).max, guaranteeing that the approval will succeed. For more information, consult the relevant OpenZeppelin SafeERC20::forceApprove implementation.

Alleviation:

A token approval of 0 is performed before each non-zero approval assignment, ensuring that the approvals will always succeed.

PEC-03M: Potentially Inaccurate Maximum Withdrawal Mechanism

Type	Severity	Location
Mathematical Operations		ProtectedERC20Gateway.sol:L68-L70, L74, L76, L103, L107, L109, L110

Description:

The ProtectedERC20Gateway::withdrawProtectedToken and ProtectedERC20Gateway::withdrawProtectedTokenWithPermit functions will assume that an amount of type(uint256).max refers to a withdrawal of the user's available balance, however, the measurement utilized as the amount withdrawn is the user's IAToken::balanceOf whereas the pool implementation will utilize the user's IAToken::scaledBalanceOf multiplied with the next liquidity index.

Impact:

While a loss arising from these mathematical inaccuracies would be inconsequential, it may lead to unfulfillable transfers in case this inaccuracy leads to an amountToWithdraw less than the expected one.

Example:

Copy
58function withdrawProtectedToken(
59  address pToken,
60  uint256 amount,
61  address to
62) external {
63  IAToken apToken = IAToken(POOL.getReserveData(pToken).aTokenAddress);
64  uint256 userBalance = apToken.balanceOf(msg.sender);
65  uint256 amountToWithdraw = amount;
66
67  // if amount is equal to uint(-1), the user wants to redeem everything
68  if (amount == type(uint256).max) {
69    amountToWithdraw = userBalance;
70  }
71  // get the aToken from user
72  apToken.transferFrom(msg.sender, address(this), amountToWithdraw);
73  // withdraw the pToken
74  POOL.withdraw(pToken, amountToWithdraw, address(this));
75  // unwrap pToken into token and send to "to"
76  IPERC20(pToken).withdrawTo(to, amountToWithdraw);
77}

Recommendation:

We advise the code to perform the following steps:

• Fetch the minimum between the userBalance and the amount from the user
• Pass in the amount argument as is to the IPool::withdraw call
• Store the result of the IPool::withdraw call and use it in the ERC20Wrapper::withdrawTo call as an argument

The above steps will guarantee that all amounts are safe to use and that they respect any mathematical truncation that may occur in between calculations.

Alleviation:

The special-purpose maximum value handling logic has been removed from the ProtectedERC20Gateway::withdrawProtectedToken and ProtectedERC20Gateway::withdrawProtectedTokenWithPermit functions, instead utilizing the provided amount directly and storing the IPool::withdraw result to be utilized in the ERC20Wrapper::withdrawTo function call.

As such, we consider this exhibit alleviated as the original described vulnerability is no longer possible.