search

code-423n4 / 2021-10-union-findings

0 stars 0 forks source link

stake function in UserManager checks for allowance, which is also done in ERC20 transferFrom #24

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

loop

Vulnerability details

Just before calling safeTransferFrom the stake function has a require statement which checks whether the allowance of the contract address is enough to transfer amount from msg.sender. The same require statement is already available in the OpenZeppelin implementation of transferFrom (and indirectly safeTransferFrom), which results in the require statement being redundant.

Impact

Calling stake is slightly more expensive on successful calls as there is an extra require statement to go through.

Proof of Concept

Require statement in stake function: https://github.com/code-423n4/2021-10-union/blob/main/contracts/user/UserManager.sol#L619-L622

Require statement in OpenZeppelin ERC20 implementation of transferFrom: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC20/ERC20Upgradeable.sol#L163

GalloDaSballo commented 3 years ago

Agree with the finding, the check for allowance is redundant