sherlock-audit / 2024-05-beefy-cowcentrated-liquidity-manager-judging

5 stars 5 forks source link

petarP1998 - Phishing_Attack #100

Closed sherlock-admin4 closed 2 months ago

sherlock-admin4 commented 2 months ago

petarP1998

medium

Phishing_Attack

Summary

The StrategyPassiveManagerVelodrome::harvest function in the contract is currently using tx.origin instead of msg.sender. This introduces a security vulnerability where an attacker could exploit the function by phishing or using malicious contracts.

Vulnerability Detail

The function StrategyPassiveManagerVelodrome::harvest utilizes tx.origin, which refers to the original external account that initiated the transaction. This can be exploited by attackers through phishing attacks or malicious contracts to trick users into unknowingly executing the harvest function.

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/main/cowcentrated-contracts/contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L433-L435

Impact

An attacker can create a malicious contract that calls the harvest function on behalf of an unsuspecting user. By using tx.origin, the contract incorrectly attributes the action to the original external account, allowing the attacker to manipulate the function and potentially harvest as if they were the legitimate caller.

More info on such attacks can be found here

Code Snippet

function harvest() external {
    _harvest(tx.origin);
}

Proof Of Concept

If we make such an attack contract:

contract Attack {
    StrategyPassiveManagerVelodrome strategy;

    constructor(address _strategy) {
        strategy = StrategyPassiveManagerVelodrome(_strategy);
    }

    function attack() public {
        strategy.harvest();
    }
}

Basically we can use phishing techniques to make anyone call the attack function which calls the harvest function. Due to the fact that the harvest function uses tx.origin then basically anyone calling the attack function will be harvesting as if they called the function.

Tool used

Manual Review

Recommendation

Replace tx.origin with msg.sender to ensure that the caller of the harvest function is correctly identified.

Updated code:

function harvest() external {
    _harvest(msg.sender);
}
sherlock-admin3 commented 2 months ago

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

z3s commented:

Invalid; It's the user's responsibility to verify the contracts they use instead of the protocol's.

DHTNS commented:

Invalid -> works as intended