search

code-423n4 / 2022-08-olympus-findings

5 stars 4 forks source link

transferFrom() method is used instead of safeTransferFrom(), #409

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/modules/VOTES.sol#L51 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L259 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L312

Vulnerability details

Vulnerability details

Details & Impact The transferFrom() method is used instead of safeTransferFrom(), presumably to save gas. I however argue that this isn’t recommended because:

1)OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible 2)Given that any NFT can be used for the call option, there are a few NFTs (here’s an example) that have logic in the onERC721Received() function, which is only triggered in the safeTransferFrom() function and not in transferFrom()

Recommended Mitigation Steps Call the safeTransferFrom() method instead of transferFrom()

File: 2022-08-olympus\src\modules\VOTES.sol 51,14: function transferFrom(

File: 2022-08-olympus\src\policies\Governance.sol 259,15: VOTES.transferFrom(msg.sender, address(this), userVotes); 312,15: VOTES.transferFrom(address(this), msg.sender, userVotes);

bahurum commented 2 years ago

duplicate of #358

fullyallocated commented 2 years ago

Duplicate of #314

0xean commented 2 years ago

closing as invalid see - https://github.com/code-423n4/2022-08-olympus-findings/issues/358#issuecomment-1249605848