The owner of RPDX Decaying Bonds is not updated on token transfers

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/decaying-bonds/RdpxDecayingBonds.sol#L36-L44
https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/decaying-bonds/RdpxDecayingBonds.sol#L122

Vulnerability details

Summary

The RdpxDecayingBonds contract keeps track of a bonds mapping with bonds information including the bond token owner. When the token is transfered, the bonds[bondId].owner value should be updated, but it isn't.

Impact

The owner value will be bricked when a token is transfered, as it can't be changed by any means.

Any integration relying on this value will make wrong trusted assumptions related to the bond token owner, potentially leading to critical issues as loss of funds, because of the importance of the owner attribute has.

Ranking it as Medium, as there is no direct loss of funds within the current scope, but bricks the contract functionality, as there is no way to fix it.

Proof of Concept

The owner attribute of the bonds mapping is set on each mint(), but its value is never updated:

  // Array of bonds
  mapping(uint256 => Bond) public bonds;

  // Structure to store the bond information
  struct Bond {
    address owner; // @audit
    uint256 expiry;
    uint256 rdpxAmount;
  }

RdpxDecayingBonds.sol#L36-L44

  function mint(
    address to,
    uint256 expiry,
    uint256 rdpxAmount
  ) external onlyRole(MINTER_ROLE) {
    _whenNotPaused();
    require(hasRole(MINTER_ROLE, msg.sender), "Caller is not a minter");
    uint256 bondId = _mintToken(to);
@>  bonds[bondId] = Bond(to, expiry, rdpxAmount); // @audit

    emit BondMinted(to, bondId, expiry, rdpxAmount);
  }

RdpxDecayingBonds.sol#L122

Coded Proof of Concept

This test shows how the owner remains the same on the bonds transfer after performing a transfer.

Add this test to the tests/RdpxDecayingBondsTest.t.sol file, and run forge test --mt "testSameOwnerOnTransfer":

  function testSameOwnerOnTransfer() public {
    // Mint a token
    rdpxDecayingBonds.mint(address(this), 1223322, 5e18);

    // The tokenId is 1
    // This can be confirmed by the fact that it changes its `ownerOf()` result on the OZ ERC721 after the transfer
    uint256 tokenId = 1;

    // Check the bond token owner before the transfer
    // Check for both via the `bonds` mapping, and the OZ `ownerOf()` function
    (address ownerBeforeTransfer,,) = rdpxDecayingBonds.bonds(tokenId);
    assertEq(ownerBeforeTransfer, address(this));
    assertEq(rdpxDecayingBonds.ownerOf(tokenId), address(this));

    // Transfer token
    rdpxDecayingBonds.safeTransferFrom(address(this), address(420), tokenId);

    // The `owner` changes on the OZ `ownerOf` function, while it remains the same on the `bonds` mapping
    (address ownerAfterTransfer,,) = rdpxDecayingBonds.bonds(tokenId);
    assertEq(ownerAfterTransfer, address(this));                        // @audit remains the same after the transfer
    assertEq(rdpxDecayingBonds.ownerOf(tokenId), address(420));
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Either update the owner value on each token transfer, or remove it, as the owner of the token is already tracked via the OZ ERC721 contract.

Assessed type

Token-Transfer

[c4-pre-sort]

bytes032 marked the issue as high quality report

[c4-pre-sort]

bytes032 marked the issue as primary issue

[c4-sponsor]

psytama (sponsor) confirmed

[GalloDaSballo]

The finding lacks a clear loss of funds, although that may be possible since ownerOf is used for internal checks

[GalloDaSballo]

The code breaks the implementation of EIP721, and may also cause losses + issues with integrations

I think Medium Severity to be appropriate

[c4-judge]

GalloDaSballo marked the issue as selected for report

[QiuhaoLi]

@GalloDaSballo Hi, could you explain why the code breaks the EIP721? As shown in the PoC, rdpxDecayingBonds.ownerOf(tokenId) is correctly set to address(420). ownerAfterTransfer (the owner field in struct Bond) is unchanged. Basically, it's an unused struct field.

[GalloDaSballo]

@GalloDaSballo Hi, could you explain why the code breaks the EIP721? As shown in the PoC, rdpxDecayingBonds.ownerOf(tokenId) is correctly set to address(420). ownerAfterTransfer (the owner field in struct Bond) is unchanged. Basically, it's an unused struct field.

Thank you for flagging, you're right that EIP721 is not broken

I believe the code to be written incorrectly and while I have considered downgrading I think Med is appropriate in this situation