sherlock-audit / 2024-01-napier-judging

9 stars 6 forks source link

xiaoming90 - Unauthorised or malicious base pool #107

Closed sherlock-admin closed 7 months ago

sherlock-admin commented 7 months ago

xiaoming90

medium

Unauthorised or malicious base pool

Summary

During the deployment of the AMM pool, there was no check being performed to ensure that the base pool was deployed by CurveTricryptoFactory.

Vulnerability Detail

Within the deploy function, Item 1 of the checklist (Line 66) stated that it should check that the Base pool is deployed by CurveTricryptoFactory.

Items 2 and 3 if the checklist has been implemented at Line 69-76. However, no check was being performed for Item 1 of the checklist within the function. As a result, an admin might intentionally or accidentally configure the base pool that is not deployed by CurveTricryptoFactory. A malicious admin might use a specially crafted base pool contract to trick users into depositing into the malicious contract.

https://github.com/sherlock-audit/2024-01-napier/blob/main/v1-pool/src/PoolFactory.sol#L50

File: PoolFactory.sol
50:     function deploy(address basePool, address underlying, PoolConfig calldata poolConfig)
..SNIP..
65:         // Checklist:
66:         // 1. Base pool must be deployed by `CurveTricryptoFactory`.
67:         // 2. Underlying asset must be the same as the underlying asset of the principal tokens.
68:         // 3. Maturity of the principal tokens must be the same.
69:         uint256 maturity = ITranche(pts[0]).maturity();
70:         if (maturity != ITranche(pts[1]).maturity() || maturity != ITranche(pts[2]).maturity()) {
71:             revert Errors.FactoryMaturityMismatch();
72:         }
73:         if (
74:             ITranche(pts[0]).underlying() != underlying || ITranche(pts[1]).underlying() != underlying
75:                 || ITranche(pts[2]).underlying() != underlying
76:         ) revert Errors.FactoryUnderlyingMismatch();

Per the contest's README page, it stated that the admin/owner is "RESTRICTED".

Q: Is the admin/owner of the protocol/contracts TRUSTED or RESTRICTED?

RESTRICTED

Per the Sherlock's judging documentation, it mentioned that:

in the case of a restricted admin, the restriction must be clearly mentioned for any issue in this category to be considered valid

For this specific issue, the restriction has been outlined in the comment on Line 66 above.

Impact

Loss of assets for the victims.

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/v1-pool/src/PoolFactory.sol#L50

Tool used

Manual Review

Recommendation

Consider combining the deployment of Curve's base pool via CurveTricryptoFactory with the deployment of the AMM pool so that the returned deployed base pool address from the CurveTricryptoFactory can be assigned to the AMM directly.

sherlock-admin commented 7 months ago

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

takarez commented:

invalid: the mistake of the admin is considered invalid according to sherlocks rule; but had the code being implemented to be like that then this would hvae been a valid issue