Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permit2 doesn't allow passing from != msg.sender #45

Closed
howlbot-integration bot opened this issue Sep 16, 2024 · 4 comments
Closed

Permit2 doesn't allow passing from != msg.sender #45

howlbot-integration bot opened this issue Sep 16, 2024 · 4 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_24_group AI based duplicate group recommendation sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/wasm_erc20.rs#L188-L245

Vulnerability details

Impact

All the permit operations will be disabled for users who have valid signatures from the token owners due to msg.sender being used instead of from in wasm_erc20::take_permit2. As a result approved users won’t be able to manage positions that they have been approved to.

Proof of Concept

Positions in Seawater are represented by ERC721 contracts with approval mechanism, that means approved users can manage positions on behalf of their owners. There are also SeawaterAMM::incrPositionPermit25468326E and swapIn, swapOut with Permit.

However operators with valid Permit signatures won’t be able to manage any positions because take_permit2 always uses msg.sender as from:

wasm_erc20.rs

pub fn encode_permit2(
    token: Address,
    max_amount: U256,
    nonce: U256,
    deadline: U256,
    to: Address,
    transfer_amount: U256,
    from: Address,
    sig: &[u8],
) -> Vec<u8> {

pub fn take_permit2(
    token: Address,
    transfer_amount: U256,
    details: Permit2Args,
) -> Result<(), Error> {
    let data = encode_permit2(
        token,
        details.max_amount,
        details.nonce,
        details.deadline,
        contract::address(),
        transfer_amount,
        msg::sender(),
        details.sig,
    );

    match RawCall::new().call(PERMIT2_ADDR, &data) {
        Ok(_) => Ok(()),
        Err(v) => Err(Error::Erc20Revert(v)),
    }
}

As a result only the owner who has permitted himself can use that functionality and can’t grant anyone else who he trusts to perform operations for his position or assets.

However the purpose of permit is to allow someone to sign approve signature, so that this signature can be used by another contract to call some function on behalf of signer.

The exact same issue has been found in PoolTogether - code-423n4/2023-07-pooltogether-findings#113

Tools Used

Manual Review

Recommended Mitigation Steps

Allow passing from address who is different from the msg.sender.

Assessed type

ERC20

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_24_group AI based duplicate group recommendation bug Something isn't working primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Sep 16, 2024
howlbot-integration bot added a commit that referenced this issue Sep 16, 2024
@af-afk af-afk added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Sep 17, 2024
@alex-ppg
Copy link

The submission details that Permit2 operations are presently solely supported when the caller themselves have signed the permit payload.

I am unsure why this would be considered a vulnerability as the user may wish to avoid executing two transactions (one to approve and one to interact) which is the entire premise the permit system was built on in the first place.

Additional security measures need to be set in place if an arbitrary from argument can be specified to avoid permit payloads of other users to be arbitrarily consumed. I am inclined to consider this a QA recommendation as an expansion of the features the system supports rather than an actual HM vulnerability or missing feature.

@c4-judge
Copy link
Contributor

alex-ppg changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Sep 23, 2024
@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Sep 23, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked the issue as grade-c

@c4-judge c4-judge reopened this Oct 7, 2024
@c4-judge c4-judge added grade-b and removed grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Oct 7, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 7, 2024

alex-ppg marked the issue as grade-b

@C4-Staff C4-Staff closed this as completed Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_24_group AI based duplicate group recommendation sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants