code-423n4 / 2023-07-tapioca-findings

15 stars 10 forks source link

TricryptoLPStrategy.setTricryptoLPGetter() fails to set the new lptoken when setting a new lpGetter, as a result, the TricryptoLPStrategy will be in the wrong state and mulfunction. #60

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/curve/TricryptoLPStrategy.sol#L150-L155

Vulnerability details

Impact

Detailed description of the impact of this finding. TricryptoLPStrategy.setTricryptoLPGetter() fails to set the new lptoken when setting a new lpGetter, as a result, the TricryptoLPStrategy will be in the wrong state and mulfunction.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

TricryptoLPStrategy.setTricryptoLPGetter() allows the owner to set a new lpGetter.

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/curve/TricryptoLPStrategy.sol#L150C14-L155

One important setup is to assign lpToken = IERC20(lpGetter.lpToken()); (see the code in the constructor). However, TricryptoLPStrategy.setTricryptoLPGetter() fails to assign a new lpToken when a new lpGetter is set. As a result, the lpToken is still old while lpGetter is new, leading a mixed state of the TricryptoLPStrategy contract, which will malfunction.

Tools Used

VSCode

Recommended Mitigation Steps

Reassign lpToken when setting a new lpGetter.

   function setTricryptoLPGetter(address _lpGetter) external onlyOwner {
        emit LPGetterSet(address(lpGetter), _lpGetter);
        wrappedNative.approve(address(lpGetter), 0);
+       lpToken = IERC20(_lpGetter.lpToken());
        lpGetter = ITricryptoLPGetter(_lpGetter);
        wrappedNative.approve(_lpGetter, type(uint256).max);
    }

Assessed type

Upgradable

c4-pre-sort commented 1 year ago

minhquanym marked the issue as primary issue

0xRektora commented 1 year ago

Duplicate of #52 and #59

c4-sponsor commented 1 year ago

cryptotechmaker marked the issue as disagree with severity

cryptotechmaker commented 1 year ago

Should be low or maximum medium severity. Replacing the property is unlikely and strategies are also replaceable. It's a duplicate as well

c4-sponsor commented 1 year ago

cryptotechmaker (sponsor) confirmed

c4-judge commented 1 year ago

dmvt marked the issue as duplicate of #52

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

dmvt marked the issue as grade-b

c4-judge commented 1 year ago

dmvt marked the issue as grade-a