FIAT DAO veFDT contest - ladboy233's results

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L236 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L242 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L247 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L250 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L278 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L321

Vulnerability details

Impact

Detailed description of the impact of this finding.

import "@openzeppelin/contracts/utils/math/SafeCast.sol"; Wrappers over Solidity’s uintXX/intXX casting operators with added overflow checks.

Downcasting from uint256/int256 in Solidity does not revert on overflow. This can easily result in undesired exploitation or bugs, since developers usually assume that overflows raise errors. SafeCast restores this intuition by reverting the transaction when such an operation overflows.

Using this library instead of the unchecked operations eliminates an entire class of bugs, so it’s recommended to use it always.

Can be combined with SafeMath and SignedSafeMath to extend it to smaller types, by performing all math on uint256 and int256 and then downcasting.

https://docs.openzeppelin.com/contracts/4.x/api/utils#SafeCast

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

In the code,

 int128(int256((iterativeTime - lastCheckpoint)));

if iterativTIme - lastCheckpoint is a number that above the range of int128, Solidity does not revert on overflow., which can cause issue.

Tools Used

Recommended Mitigation Steps

can use SafeCast and StableMath from oppezeppelin

import { SafeCast } from "@openzeppelin/contracts/utils/math/SafeCast.sol";

and instead of

unsafe casting

 int128(int256((iterativeTime - lastCheckpoint)));

We can use

SafeCast.toInt128(int256(MAXTIME));

USE A MORE RECENT VERSION OF SOLIDITY

Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

the blocklist.sol and VotingEscrow all use solidty version 0.8.3

Use Openzeppelin ownable to manage the ownership.

Use https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol to manage the ownership save from passing owner into constructor and use require to check in every critcal function.

We can just use onlyOwner modifier.

Use SafeERC20 instead of IERC20 interface.

safeERC20 take care of transfer return value check so

 require(token.transfer(msg.sender, value), "Transfer failed");

in lines below is no longer needed.

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L676

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L546

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L657

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L767

is no longer needed.

function can be marked as external

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/features/Blocklist.sol#L33

isBlocked can be marked as external

USING BOOLS FOR STORAGE INCURS OVERHEAD

// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L10

Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

using uint256 also save efforts from integre downcasting.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L70

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L71

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L76

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L78