xiaoming90 - `withdraw` function does not comply with ERC5095

[sherlock-admin2]

xiaoming90

medium

withdraw function does not comply with ERC5095

Summary

The withdraw function of Tranche/PT does not comply with ERC5095 as it does not return the exact amount of underlying assets requested by the users.

Vulnerability Detail

Per the contest's README page, it stated that the code is expected to comply with ERC5095 (https://eips.ethereum.org/EIPS/eip-5095). As such, any non-compliance to ERC5095 found during the contest is considered valid.

Q: Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of? EIP20 and IERC5095

Following is the specification of the withdraw function of ERC5095. It stated that the user must receive exactly underlyingAmount of underlying tokens.

withdraw Burns principalAmount from holder and sends exactly underlyingAmount of underlying tokens to receiver.

However, the withdraw function does not comply with this requirement.

On a high-level, the reason is that Line 337 will compute the number of shares that need to be redeemed to receive underlyingAmount number of underlying tokens from the adaptor. The main problem here is that the division done here is rounded down. Thus, the sharesRedeem will be lower than expected. Consequently, when sharesRedeem number of shares are redeemed at Line 346 below, the users will not receive an exact number of underlyingAmount of underlying tokens.

https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/Tranche.sol#L328

File: Tranche.sol
328:     function withdraw(
329:         uint256 underlyingAmount,
330:         address to,
331:         address from
332:     ) external override nonReentrant expired returns (uint256) {
333:         GlobalScales memory _gscales = gscales;
334:         uint256 cscale = _updateGlobalScalesCache(_gscales);
335: 
336:         // Compute the shares to be redeemed
337:         uint256 sharesRedeem = underlyingAmount.divWadDown(cscale);
338:         uint256 principalAmount = _computePrincipalTokenRedeemed(_gscales, sharesRedeem);
339: 
340:         // Update the global scales
341:         gscales = _gscales;
342:         // Burn PT tokens from `from`
343:         _burnFrom(from, principalAmount);
344:         // Withdraw underlying tokens from the adapter and transfer them to `to`
345:         _target.safeTransfer(address(adapter), sharesRedeem);
346:         (uint256 underlyingWithdrawn, ) = adapter.prefundedRedeem(to);

Impact

The tranche/PT does not align with the ERC5095 specification.

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/Tranche.sol#L328

Tool used

Manual Review

Recommendation

Update the withdraw function to send exactly underlyingAmount number of underlying tokens to the caller so that the Tranche will be aligned with the ERC5095 specification.

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid: medium(4)

[sherlock-admin3]

The protocol team fixed this issue in PR/commit https://github.com/napierfi/napier-v1/pull/170.

[massun-onibakuchi]

@nevillehuang I think this finding severity can be much lower. Actually EIP5095 is stagnant not even final status, which means it could be changed. we have not known protocols which uses this standard.

https://eips.ethereum.org/

[Banditx0x]

Escalate

This is low/informational. Zero impact, does not break core functionality, or any functionality for that matter.

[sherlock-admin2]

Escalate

This is low/informational. Zero impact, does not break core functionality, or any functionality for that matter.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

@Banditx0x Seems to be right according to this sherlock rule:

EIP Compliance: For issues related to EIP compliance, the protocol & codebase must show that there are important external integrations that would require strong compliance with the EIP's implemented in the code. The EIP must be in regular use or in the final state for EIP implementation issues to be considered valid

[xiaoming9090]

Per the contest's README page, the protocol explicitly states that the code is expected to comply with ERC5095 (https://eips.ethereum.org/EIPS/eip-5095). Thus, any non-compliance to ERC5095 found by the Watsons during the contest is considered a valid Medium.

Otherwise, it would be unfair to all the Watson who made an effort to verify if the contracts comply with ERC5095.

Q: Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of? EIP20 and IERC5095

Note that the Hierarchy of truth in Sherlock's contest, "Contest README", precedes everything else, including "Sherlock rules for valid issues"

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel.

[Banditx0x]

There are plenty of informational/low issues that don't get rewarded. The mention of an EIP in the ReadMe does not automatically boost an issue to Medium.

[cvetanovv]

For me, this issue is borderline medium/low. In the past, such reports have been Medium, but I also agree with @Banditx0x comment that this is more like Low severity.

[Czar102]

I am inclined to leave the issue as is, but will consult team members before sharing a preliminary verdict.

[Czar102]

After discussing this, I'm planning to reject the escalation and leave the issue as is. It will be made more explicit in the rules that issues about mentioned EIPs are valid.

[Czar102]

Result: Medium Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• banditx0x: rejected

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.