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

Transferring bond doesn't update owner #532

Closed
code423n4 opened this issue Aug 31, 2023 · 4 comments
Closed

Transferring bond doesn't update owner #532

code423n4 opened this issue Aug 31, 2023 · 4 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 duplicate-1030 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@code423n4
Copy link
Contributor

code423n4 commented Aug 31, 2023

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/decaying-bonds/RdpxDecayingBonds.sol#L162

Vulnerability details

Impact

Detailed description of the impact of this finding.
Transferring bond doesn't update owner

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
Decayed bonds are meant to be transferring(exchanging) due to the fact that there is a function that forbid to transfer them if bonds in a pause state.

  function _beforeTokenTransfer(
    address from,
    address to,
    uint256 tokenId,
    uint256 batchSize
  ) internal override(ERC721, ERC721Enumerable) {
    _whenNotPaused();
    super._beforeTokenTransfer(from, to, tokenId, batchSize);
  }

RdpxDecayingBonds.sol#L162

But whenever user trasferring their bonds he still holds power and all bonuses of that bond due to the fact that bonds[bondId].to stays the same.

Tools Used

POC

  function testMint1() public {
    // Mint 1000 RDPX bonds
    rdpxDecayingBonds.mint(address(this), 1223322, 5e18);

    // Check the bond details
    (address owner,  ,) = rdpxDecayingBonds
      .bonds(1);

    console.log("nft owner before:   ", rdpxDecayingBonds.ownerOf(1));
    console.log("bonds owner    before:   ", owner);
    rdpxDecayingBonds.transferFrom(address(this), address(1), 1);
    (owner,, ) = rdpxDecayingBonds.bonds(1);
    console.log("nft owner after: ", rdpxDecayingBonds.ownerOf(1));
    console.log("bonds owner after: ", owner);
  }
  nft owner before:    0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
  bonds owner    before:    0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
  nft owner after:  0x0000000000000000000000000000000000000001
  bonds owner after:  0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496

Recommended Mitigation Steps

  function _beforeTokenTransfer(
    address from,
    address to,
    uint256 tokenId,
    uint256 batchSize
  ) internal override(ERC721, ERC721Enumerable) {
    _whenNotPaused();
+   bonds[tokenId].owner = to;
    super._beforeTokenTransfer(from, to, tokenId, batchSize);
  }

Assessed type

Error

@code423n4 code423n4 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 Aug 31, 2023
code423n4 added a commit that referenced this issue Aug 31, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Sep 9, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as primary issue

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Sep 9, 2023
@c4-pre-sort c4-pre-sort added duplicate-1030 and removed primary issue Highest quality submission among a set of duplicates labels Sep 15, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as duplicate of #1030

@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Oct 18, 2023
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 duplicate-1030 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants