ProtectedNativeTokenGateway Manual Review Findings

PNT-01M: Inexistent Renewal of Approvals

Type	Severity	Location
Standard Conformity		ProtectedNativeTokenGateway.sol:L53-L54

Description:

In contrast to the ProtectedERC20Gateway::depositProtectedToken function, the ProtectedNativeTokenGateway::depositProtectedBNB function will not automatically renew its WBNB and pWBNB approvals.

Example:

Copy
48function depositProtectedBNB(
49  address onBehalfOf,
50  uint16 referralCode
51) external payable {
52  WBNB.deposit{value: msg.value}();
53  pWBNB.depositFor(address(this), msg.value);
54  POOL.deposit(address(pWBNB), msg.value, onBehalfOf, referralCode);
55}

Recommendation:

We advise the ProtectedNativeTokenGateway::depositProtectedBNB function to align with the ProtectedERC20Gateway::depositProtectedToken function, refreshing its approvals whenever necessary.

Alleviation:

The Kinza Finance team has stated that the allowance being set in the contract's ProtectedNativeTokenGateway::constructor is sufficient for the lifetime of the contract and as such they do not wish to introduce an approval refresh mechanism.

As such, we consider this exhibit nullified as the Kinza Finance team has assessed the current behaviour is sufficient.

PNT-02M: Potential Withdrawal Denial-of-Service

Type	Severity	Location
Language Specific		ProtectedNativeTokenGateway.sol:L104

Description:

The ProtectedNativeTokenGateway::withdrawBNBWithPermit 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 ProtectedNativeTokenGateway::withdrawBNBWithPermit 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
87function withdrawBNBWithPermit(
88  uint256 amount,
89  address to,
90  uint256 deadline,
91  uint8 permitV,
92  bytes32 permitR,
93  bytes32 permitS
94) external {
95  IAToken apWBNB = IAToken(POOL.getReserveData(address(pWBNB)).aTokenAddress);
96  uint256 userBalance = apWBNB.balanceOf(msg.sender);
97  uint256 amountToWithdraw = amount;
98
99  // if amount is equal to type(uint256).max, the user wants to redeem everything
100  if (amount == type(uint256).max) {
101    amountToWithdraw = userBalance;
102  }
103  // permit `amount` rather than `amountToWithdraw` to make it easier for front-ends and integrators
104  apWBNB.permit(msg.sender, address(this), amount, deadline, permitV, permitR, permitS);
105  apWBNB.transferFrom(msg.sender, address(this), amountToWithdraw);
106  POOL.withdraw(address(pWBNB), amountToWithdraw, address(this));
107  // withdraw the pToken to wToken first
108  pWBNB.withdrawTo(address(this), amountToWithdraw);
109  // withdraw the wToken to native token
110  WBNB.withdraw(amountToWithdraw);
111  _safeTransferBNB(to, amountToWithdraw);
112}

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.

PNT-03M: Potentially Inaccurate Maximum Withdrawal Mechanism

Type	Severity	Location
Mathematical Operations		ProtectedNativeTokenGateway.sol:L63, L72, L75, L77, L79, L81, L83, L88, L101, L105, L106, L108, L110, L111

Description:

The ProtectedNativeTokenGateway::withdrawProtectedBNB and ProtectedNativeTokenGateway::withdrawBNBWithPermit 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
62function withdrawProtectedBNB(
63  uint256 amount,
64  address to
65) external {
66  IAToken apWBNB = IAToken(POOL.getReserveData(address(pWBNB)).aTokenAddress);
67  uint256 userBalance = apWBNB.balanceOf(msg.sender);
68  uint256 amountToWithdraw = amount;
69
70  // if amount is equal to uint(-1), the user wants to redeem everything
71  if (amount == type(uint256).max) {
72    amountToWithdraw = userBalance;
73  }
74  // transfer aPToken from msg.sender to this contract
75  apWBNB.transferFrom(msg.sender, address(this), amountToWithdraw);
76  // withdraw aPToken to pToken
77  POOL.withdraw(address(apWBNB), amountToWithdraw, address(this));
78  // withdraw the pToken to wToken first
79  pWBNB.withdrawTo(address(this), amountToWithdraw);
80  // withdraw the wToken to native token
81  WBNB.withdraw(amountToWithdraw);
82  // transfer BNB back to the caller 
83  _safeTransferBNB(to, amountToWithdraw);
84}

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 ProtectedNativeTokenGateway::withdrawProtectedBNB and ProtectedNativeTokenGateway::withdrawBNBWithPermit 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.