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 #246

Open
c4-bot-1 opened this issue Sep 13, 2024 · 0 comments
Open

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

c4-bot-1 opened this issue Sep 13, 2024 · 0 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working 🤖_24_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@c4-bot-1
Copy link
Contributor

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

@c4-bot-1 c4-bot-1 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Sep 13, 2024
c4-bot-4 added a commit that referenced this issue Sep 13, 2024
@c4-bot-12 c4-bot-12 added the 🤖_24_group AI based duplicate group recommendation label Sep 13, 2024
howlbot-integration bot added a commit that referenced this issue Sep 16, 2024
@howlbot-integration howlbot-integration bot added the sufficient quality report This report is of sufficient quality label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working 🤖_24_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

2 participants